Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show JTree children in correct order #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pfroud
Copy link

@pfroud pfroud commented Apr 14, 2022

The GUI uses the getDeclaredMethods() method to populate JTree children. But the Javadoc says:

The elements in the returned array are not sorted and are not in any particular order.

So the order of nodes in the JTree sometimes does not match the order in the KSY file.

Here's an example using png.ksy (from ide.kaitai.io). The order in the KSY file:

seq:
  - id: magic
  - id: ihdr_len
  - id: ihdr_type
  - id: ihdr
  - id: ihdr_crc
  - id: chunks

Example of the GUI showing the wrong order:
jtree-order-wrong

This pull request gets the correct order from the _seqFields field, which is generated from the KSY file.

The _seqFields field in the compiled png.java file:

public static String[] _seqFields = new String[] { "magic", "ihdrLen", "ihdrType", "ihdr", "ihdrCrc", "chunks" };

Now the JTree nodes are in the correct order:
jtree-order-correct

The class.getDeclaredMethods() method returns methods in an arbitrary order. Use the generated _seqFields field instead.
Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid that it's not possible to rely on SEQ_FIELDS alone, because the instances won't be found this way. The existing implementation iterated over methods, where instances were available, so showing only seq fields now would be an ugly regression.

Or am I missing something?

src/main/java/io/kaitai/struct/visualizer/DataNode.java Outdated Show resolved Hide resolved
@pfroud
Copy link
Author

pfroud commented Apr 16, 2022

You are not missing something, it doesn't work with instances!!!

KSY file to test it:

meta:
  id: instances_test
seq:
  - id: my_sequential_item
    type: u1
instances:
  my_instance:
    pos: 1
    type: u1

In the compiled Java code, the seqFields line does not have the instance:

public static String[] _seqFields = new String[] { "mySequentialItem" };

But the methods let us access the sequential item and the instance:

public long mySequentialItem() { ... }
public Long myInstance() { ... }

What do you think about this:

  • Find sequential items using seq_fields. The order of the sequential items will match the order in the KSY file.
  • Find instances by calling getDeclaredMethods() and then excluding names that are in seq_fields. The instances will be in arbitrary order. Show the instances in the JTree after all the sequential items.

@generalmimon
Copy link
Member

@pfroud:

What do you think about this:

  • Find sequential items using seq_fields. The order of the sequential items will match the order in the KSY file.
  • Find instances by calling getDeclaredMethods() and then excluding names that are in seq_fields. The instances will be in arbitrary order. Show the instances in the JTree after all the sequential items.

Yes, seems reasonable to me. Order of instances in the .ksy spec isn't guaranteed anyway, because in the YAML tree they're defined in a mapping, where the key order is unspecified (each key is the identifier of a particular instance, equivalent of the id key in a seq field) - see the YAML 1.2 spec for reference. So not even the KS compiler knows what the "correct" order of instances should be - it doesn't matter.

@pfroud pfroud requested a review from generalmimon April 16, 2022 18:22
@pfroud
Copy link
Author

pfroud commented Apr 16, 2022

OK, I implemented that change, let me know how it looks

Screenshot using the minimal testing KSY file:
instances test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants