Concise creation of simple DynamicTableColumns #3
Conversation
|
I have created a ton of columns over the years. I have never felt them to be too verbose. This is probably because I would put them at the bottom of the file, out of the way until needed. By having them as full classes, debugging and development are a bit easier. That being said, in your case, it seems like you are creating these tables quickly and perhaps throwing them away later. In this case, I can see the verbosity being annoying. We do have another way of building tables, but it is quite a bit simpler. You get smaller code, but perhaps at the expense of less functionality. This class is the https://github.com/NationalSecurityAgency/ghidra/blob/a1f243ebbab2fbad75cdd6e76b463902523b4ead/Ghidra/Framework/Docking/src/main/java/docking/widgets/table/GTableWidget.java. It was designed to take a row object type and a list of method names on that type that should be used to create columns for the user. I'm curious if you have seen and/or used that class. |
do you have an example where this is easier? I'm using IntelliJ, so maybe that's something that just differs by IDE, but I can set breakpoints inside the anonymous accessor (if I provide one instead of a method reference), and I'm not sure what other debug/development scenarios would come up where it's relevant to have a standalone class
The tables are for plugins that will hopefully not be thrown away, but the tables are often simple and most of the code ends up being column definitions. Given that each column takes ~10-12 lines of code to define, this quickly blows up. E.g. I have a tableModel with 8 columns, and most of the code in the file is just the column definitions. A few of these plugins are also projects with other developers that are not that familiar with Ghidra. I think especially in those cases there is a lot of benefit from keeping the code concise so it only concerns the actually important information, instead of the same boiler plate code ten times with subtly different types. |
Perhaps it is more of a code organization preference. Also, we have some columns that contain more business logic that in your example. The more code for a column, the more it makes sense to be in its own class, IMO. We do this in most of our tables. You can see an example in: Even when the business logic is not large, we sometimes have to add custom rendering or filtering logic. For simple tables, perhaps this is not needed. Honestly, we have just kind of always followed this pattern, with the setup inside of |
|
I changed the API a bit to also support adding hidden tables. From my side I would be happy if this gets merged in it's current state, but I can also incorporate changes to the signature e.g. how to control the visibility, adding parameter for to override |
|
Warning Rate limit exceeded@visz11 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change adds two overloaded Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableColumnDescriptor
participant RowObjectAccessor
participant AbstractDynamicTableColumn
User->>TableColumnDescriptor: addColumn(name, columnTypeClass, RowObjectAccessor, [visible])
TableColumnDescriptor->>AbstractDynamicTableColumn: Create anonymous subclass with accessor
AbstractDynamicTableColumn->>RowObjectAccessor: getValue(rowObject)
TableColumnDescriptor->>TableColumnDescriptor: Add column (visible or hidden)
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Ghidra/Framework/Docking/src/main/java/docking/widgets/table/TableColumnDescriptor.java (1)
100-136: Implementation properly enables concise column creationThe new
addColumnmethod successfully enables a more concise way to define table columns using functional interfaces rather than anonymous classes. The implementation correctly handles creating the underlying column object and setting its visibility.I have a few notes on the implementation:
- The
getSupportedRowType()method returns null due to type erasure limitations, which is noted in the comments. This could potentially affect code that relies on this information.- The accessor function only receives the row object parameter, but does not have access to the
settings,data, orserviceProviderparameters that are available in the full column implementation.Consider enhancing the implementation to allow overriding additional column properties such as custom renderers, tooltips, or comparators. For example:
public <COLUMN_TYPE> void addColumn( String name, Class<COLUMN_TYPE> columnTypeClass, RowObjectAccessor<ROW_TYPE, COLUMN_TYPE> rowObjectAccessor, boolean visible, ColumnProperties<ROW_TYPE, COLUMN_TYPE> properties) { // Implementation with optional properties } // Optional properties class public class ColumnProperties<ROW_TYPE, COLUMN_TYPE> { private Comparator<COLUMN_TYPE> comparator; private TableCellRenderer renderer; // Additional properties // Builder methods public ColumnProperties<ROW_TYPE, COLUMN_TYPE> withComparator(Comparator<COLUMN_TYPE> comparator) { this.comparator = comparator; return this; } // Other builder methods }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Ghidra/Framework/Docking/src/main/java/docking/widgets/table/TableColumnDescriptor.java(3 hunks)
🔇 Additional comments (3)
Ghidra/Framework/Docking/src/main/java/docking/widgets/table/TableColumnDescriptor.java (3)
20-21: New imports are correctly addedThe addition of these imports is necessary to support the new method signatures in the
AbstractDynamicTableColumnimplementation.
137-146: Convenience method correctly defaults to visible columnsThis overloaded method provides a useful shortcut for the common case of adding visible columns.
183-192:✅ Verification successful
Functional interface is appropriately defined
The
RowObjectAccessorfunctional interface is well-designed with appropriate generic type parameters to maintain type safety between row objects and column values. The@FunctionalInterfaceannotation correctly indicates that this is intended to be used with lambda expressions or method references.The current interface only supports accessing row data directly and throws
IllegalArgumentException. Verify whether columns in your application commonly need access toSettingsorServiceProviderparameters. If they do, consider adding an alternative accessor interface:
🏁 Script executed:
#!/bin/bash # Count how many column implementations use Settings or ServiceProvider in getValue echo "Searching for columns that use Settings in their getValue method..." rg -A 5 "getValue.*Settings.*\{" --type java echo "Searching for columns that use ServiceProvider in their getValue method..." rg -A 5 "getValue.*ServiceProvider.*\{" --type javaLength of output: 69073
RowObjectAccessor interface is sufficient as‑is
A search for table column accessors referencing Settings or ServiceProvider turned up no addColumn() usages capturing those parameters. While many DataType
getValuemethods take a Settings argument, none of the table column lambdas or implementations require Settings or ServiceProvider in their row‑value accessors. No additional functional interface is needed at this time.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This is a feature proposal and proof of concept to allow concise definition of DynamicTableColumns
I have been working a lot with tables recently where I use a Java record as a
ROW_TYPEand want to quickly define the columns. E.g. for a example recordCurrently, to my best knowledge, the best way to now create the
TableColumnDescriptorfor this is:This PR adds an extra PoC method to
TableColumnDescriptorthat allows achieving the same in significantly less lines, and with just the important information:The second argument that explicitly specifies is needed because the type information will otherwise be lost at runtime. I consider this a small price to pay in terms of verbosity, as it is still significantly less lines than by default, and the Compiler still enforces that this type matches the return value of the accessor, so there is no risk of accidentally mixing types AFAIU.
I'd first like feedback if this approach has any drawbacks I'm not aware of, or if there is a better way to add
TableColumns that I missed. If this is a worthwhile addition I can extend the extra methods to also cover the cases for hidden columns, and potentially sorting. It should also be possible to add a method that takes an accessor function that has the full signaturepublic COLUMN_TYPE getValue(ROW_TYPE rowObject, Settings settings, Object data, ServiceProvider serviceProvider)instead of justpublic COLUMN_TYPE getValue(ROW_TYPE rowObject)if this is considered worthwhile.Summary by CodeRabbit