Concise creation of simple DynamicTableColumns

[fmagin]

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_TYPE and want to quickly define the columns. E.g. for a example record

public record FunctionWithExtraInfo(Function func, String extraText, Integer someMagicNumber) {}

Currently, to my best knowledge, the best way to now create the TableColumnDescriptor for this is:

	protected TableColumnDescriptor<FunctionWithExtraInfo> createTableColumnDescriptor() {
		TableColumnDescriptor<FunctionWithExtraInfo> descriptor = new TableColumnDescriptor<>();

                descriptor.addVisibleColumn(new AbstractDynamicTableColumn<FunctionWithExtraInfo, Function, Object>(){

			@Override
			public String getColumnName() {
				return "Function";
			}

			@Override
			public Function getValue(FunctionWithExtraInfo rowObject, Settings settings, Object data, ServiceProvider serviceProvider) throws IllegalArgumentException {
				return rowObject.func()
			}
		});

		descriptor.addVisibleColumn(new AbstractDynamicTableColumn<FunctionWithExtraInfo, String, Object>(){

			@Override
			public String getColumnName() {
				return "Extra Text";
			}

			@Override
			public String getValue(FunctionWithExtraInfo rowObject, Settings settings, Object data, ServiceProvider serviceProvider) throws IllegalArgumentException {
				return rowObject.extraText();
			}
		});
                descriptor.addVisibleColumn(new AbstractDynamicTableColumn<FunctionWithExtraInfo, Integer, Object>(){

			@Override
			public String getColumnName() {
				return "Magic Number";
			}

			@Override
			public Integer getValue(FunctionWithExtraInfo rowObject, Settings settings, Object data, ServiceProvider serviceProvider) throws IllegalArgumentException {
				return rowObject.magicNumber()
			}
		});
}

This PR adds an extra PoC method to TableColumnDescriptor that allows achieving the same in significantly less lines, and with just the important information:

protected TableColumnDescriptor<FunctionWithExtraInfo> createTableColumnDescriptor() {
		TableColumnDescriptor<FunctionWithExtraInfo> descriptor = new TableColumnDescriptor<>();

                descriptor.addVisibleColumn("Function", Function.class, FunctionWithExtraInfo::func);
                descriptor.addVisibleColumn("Extra Info", String.class, FunctionWithExtraInfo::extraInfo);
                descriptor.addVisibleColumn("Magic Number", Integer.class, FunctionWithExtraInfo::magicNumber);
}

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 signature public COLUMN_TYPE getValue(ROW_TYPE rowObject, Settings settings, Object data, ServiceProvider serviceProvider) instead of just public COLUMN_TYPE getValue(ROW_TYPE rowObject) if this is considered worthwhile.

[dragonmacher]

Just looking at this and exploring thoughts that come to mind. It seems like at the point your create the column, the passed in rowObjectAccessor would still have the type information. This code was written long before Java method references existed. At line 112, as the column is getting created, does the given method reference still have its type information? Your comment below implies it does not. If it still had this info, then the signature could be very simple, with just 2 arguments and we would still have the row and column types.

[fmagin]

I tried this, at least with my version that had this snippet in an external file, the type information was already lost inside the method that accepted the RowObjectAccessor<ROW_TYPE, COLUMN_TYPE> rowObjectAccessor as the argument. I don't have a deep enough understanding of how exactly type erasure works in Java, so I can't make arguments when it is still available and when it's not available. And after chasing down this bug in the first place (getting a NPE elsewhere, when trying to open a filter), I am appreciating the solution that just does not involve reflection anywhere in the first place and is easier to reason about.

[fmagin]

Ah, it might work when passing method references to existing functions, but I think it definitely doesn't work when passing anonymous functions (which is needed for accessing nested objects):

addVisibleColumn("Nested Info", String.class, (row) -> row.someNestedRecord().field())

[dragonmacher]

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.

[fmagin]

having them as full classes, debugging and development are a bit easier

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

That being said, in your case, it seems like you are creating these tables quickly and perhaps throwing them away later.

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.
When I first started using the tables this took me a while to realize because it was hard to discern what was actually going on.

[dragonmacher]

do you have an example where this is easier?

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:

https://github.com/NationalSecurityAgency/ghidra/blob/a1f243ebbab2fbad75cdd6e76b463902523b4ead/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symtable/AbstractSymbolTableModel.java

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 createTableColumnDescriptor() being as simple as adding the classes you want to be in the descriptor. Since we have never done it any other way, we have not considered adding more methods to the descriptor. That being said, your use case makes sense to me.

[fmagin]

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 getColumnDescription, or other simple things I might have missed.