scijava / scijava-common

A plugin framework and application container with built-in extensibility mechanism :electric_plug:
BSD 2-Clause "Simplified" License
87 stars 52 forks source link

Widget grouping #310

Open tischi opened 6 years ago

tischi commented 6 years ago

Making it possible to group IJ2 plugin widgets, e.g. horizontally or in tabs, would be great in order to structure the UI in case of many inputs. See also: http://forum.imagej.net/t/any-control-over-parameterized-input-field-position-layout/1554/5

imagejan commented 6 years ago

For choices, there is radioButtonHorizontal already:

#@ String (choices={"a", "b", "c"}, style="radioButtonHorizontal") myChoice

(I would also love to see multiple-select (https://github.com/scijava/scijava-common/issues/260) creating a list of checkboxes in this context.)


How would you imagine a possible syntax to create horizontal or tab groups? Something like this:

#@ Boolean (style="group-horizontal", group="first") a
#@ String (style="group-horizontal", group="first") b
#@ Boolean (style="group-horizontal", group="second") c
#@ String (style="group-horizontal", group="second") d
#@ String (style="group-tab", group="other") e

?

How would you decide how to render things if not all parameters have group annotations? All unannotated parameters go into the "main" dialog?


Another option (at least in scripts) would be to define tab groups in a new line with a dedicated script directive, but I don't know how the same would be nicely achievable in Java plugins then...

tischi commented 6 years ago

Your syntax with (style="group-horizontal", group="first") looks sensible to me.

Horizontal grouping

I guess one could still keep the logic that the parameters appear vertically in the same order as in the code (for groups I would take the first appearance of the respective group as the order). For example:

#@ Boolean (style="group-horizontal", group="first") a
#@ Boolean (style="group-horizontal", group="second") c
#@ String e
#@ String (style="group-horizontal", group="second") d
#@ String (style="group-horizontal", group="first") b

Would appear in this order

a    b
c    d
e

Tabs

I think, for starters, maybe it would be good to just implement first the horizontal grouping idea and leave out the tab idea for now. What do you think?

imagejan commented 6 years ago

I think, for starters, maybe it would be good to just implement first the horizontal grouping idea and leave out the tab idea for now. What do you think?

Seems sensible.


So here's a (likely incomplete) list of things to be done:

@ctrueden, @bnorthan, @nicost Does that sound reasonable?

hadim commented 6 years ago

Just checking here if any progress has been made on that front?

ctrueden commented 6 years ago

Sorry @hadim, I have not worked on it yet. I will be working on the next major version of SciJava Common over the summer.

skalarproduktraum commented 3 years ago

Hi everyone, sorry for being late to the party, @imagejan has just pointed me to this. I was in need for such functionality for sciview, here's what this looks like:

image

The way this is implemented is adding a group: attribute to @Parameter's style (thanks again to @imagejan for pointing style out to me, I had (ab)used description before, style is a much better place for this). My ideal solution would have been to use Attr for that, but unfortunately, DefaultMutableModuleItem does not have access to attrs(), while CommandModuleItem does and I did not find an easy solutinon or workaround for this.

Now we need this only in the inspector panel so far, so I have created a new input harvester, called the SwingGroupingInputHarvester. The only changes this one has compared to the regular SwingInputHarvester is that it groups the inputs by the above group: attribute, and creates a collapsible JPanel for each of the members.

As said I didn't know about this discussion here, I'm happy to PR this after I2K 👍

imagesc-bot commented 3 years ago

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/access-to-imagej2-input-harvester-generated-elements/49338/5

NicoKiaru commented 3 years ago

Can't wait! Where's the PR @skalarproduktraum ;-) ?

karlduderstadt commented 3 years ago

@skalarproduktraum This is a really cool idea. My dialogs are in desperate need of groups. I guess in your implementation you directly build the panel with the SwingGroupingInputHarvester. I went ahead and tried to make a draft of this to integrate into scijava-ui-swing, which is currently on a branch of my fork - https://github.com/karlduderstadt/scijava-ui-swing/tree/group_dialog_inputs actually all changes are currently in one commit - https://github.com/karlduderstadt/scijava-ui-swing/commit/1fa38cd08ff381905aaaf00ea527b8ab30b9bd36

This lead to several questions about how best to implement it. Seemed like the best option was to change the SwingInputHarvester to detect the group style and in that case use a new class for building the input panel, which I called SwingGroupedInputPanel (https://github.com/karlduderstadt/scijava-ui-swing/blob/group_dialog_inputs/src/main/java/org/scijava/ui/swing/widget/SwingGroupedInputPanel.java).

I am having trouble testing it though because in legacy UI mode, which I was using, I guess the SwingInputHarvester is not used ? Or is called from somewhere else? I am pretty confused about this. I don't have more time to investigate now.

ctrueden commented 3 years ago

in legacy UI mode, which I was using, I guess the SwingInputHarvester is not used ?

It is still used, because LegacyUI implements the SwingUI interface, so the legacy user interface is still treated as a Swing one, for the purpose of things like the SwingInputHarvester.

Seemed like the best option was to change the SwingInputHarvester to detect the group style and in that case use a new class for building the input panel, which I called SwingGroupedInputPanel

My plan years ago for adding group support was to add a group field to ModuleItem, and also to the Parameter annotation interface. Then improve the standard SwingInputPanel et al. to check for groups and respect them. Needing a second InputPanel implementation would be unfortunate, although I'm open to it if you really don't see any simpler way.

karlduderstadt commented 3 years ago

Thanks for the helpful feedback! Aww that makes sense. I assumed it was something like that since clearly the SwingInputHarvester is used.

Adding a group field to ModuleItem and a Parameter annotation sounds like a better plan. I agree having an additional panel is not ideal and putting SwingInputPanels inside SwingInputPanels doesn't really seem to make a lot of sense. I will work on the strategy you suggested, adding getWidgetGroup() to ModuleItem etc., improving SwingInputPanel and see how it goes.

I do like the collapsible group labels that @skalarproduktraum developed. It might actually be nice if it was possible to specify if groups should be expanded or collapsed in commands by default and if user changes were persisted when running a command a couple times. I was thinking this could be achieved by adding group labels as boolean parameters and then setting something like visibility = ItemVisibility.GROUPNAME. But the mechanics seem a bit awkward. Is there a way group status (expanded or collapsed) can be persisted without having to add a group label parameter?

I am assuming in your proposal, the group name is only specified in the Parameter annotations of group members, and there is no separate group title/label parameter needed.

ctrueden commented 3 years ago

It might actually be nice if it was possible to specify if groups should be expanded or collapsed in commands by default

Good idea. One solution that comes to mind, which would be relatively extensible, would be support some hints in brackets after the group name. So you could do e.g.:

@Parameter(group = "Basic configuration", label = "Number of ducks")
private int duckCount = 1;

@Parameter(group = "Basic configuration", label = "Time of next feeding")
private Date nextFeeding = new Date();

@Parameter(group = "Advanced options[collapsed]", label = "Advanced duck typing")
private boolean advancedDuckTyping = true;

@Parameter(group = "Advanced options", label = "Starvation threshold")
private int starvationThreshold = 6;

Where [collapsed] is a shorthand for [collapsed=true]. Such comma-separated key=value pairs can be parsed easily using Parsington. Of course, the code could do something inconsistent like collapsed=true for one parameter and collapsed=false for another, but we could just have a "last setting wins" rule, which would be easy to code.

Or here's another idea:

@ParameterGroup
private String basic = "Basic configuration";

@ParameterGroup(expanded = false)
private String advanced = "Advanced options";

@Parameter(group = "basic", label = "Number of ducks")
private int duckCount = 1;

@Parameter(group = "basic", label = "Time of next feeding")
private Date nextFeeding = new Date();

@Parameter(group = "advanced", label = "Advanced duck typing")
private boolean advancedDuckTyping = true;

@Parameter(group = "advanced", label = "Starvation threshold")
private int starvationThreshold = 6;

Any better ideas?

karlduderstadt commented 3 years ago

I really like the second idea. That appears to satisfy the key requirements. Then we would have full control over the default state. Also, having a separate ParameterGroup annotation seems to make things very clear. As a fall back, if no ParameterGroup is specified but there is a group specified in other input annotations we could still create the group using the name provided. I will see if I can add it.

Just one question for now, would the expanded setting persist based on user changes?

If, for example, I run the command and expand all collapsed groups, will they remain expanded the next time I run the command. I know variable values persist, but I guess these options provided within the parameter don't. I suppose this is not an absolute requirement, but it would be nice.

ctrueden commented 3 years ago

If, for example, I run the command and expand all collapsed groups, will they remain expanded the next time I run the command. I know variable values persist, but I guess these options provided within the parameter don't. I suppose this is not an absolute requirement, but it would be nice.

It will not happen automatically. But you could code the groups logic to do it. You can use the PrefService to persist whatever information you want. Happy to help with this part after we have an initial version working.

karlduderstadt commented 3 years ago

Alright, I had a couple minutes so I started by adding group to Parameter annotations - https://github.com/karlduderstadt/scijava-common/commit/1949891258de0c7b6399919c6542572a769b8718

Then I started adding ParameterGroup and I realized this might require a lot more changes. For example, in CommandModuleItem the getParameter() method is called everywhere and all depends on a normal Parameter annotation. Would it be ok to just add ItemVisibility.GROUP and use normal Parameters instead of adding ParameterGroup. Then it can nicely be treated as just another input, like all the others. I would then just add expanded as yet another setting in Parameter. It already has min and max that are only used for certain inputs.

@Parameter(visibility = ItemVisibility.GROUP)
private String basic = "Basic configuration";

@Parameter(visibility = ItemVisibility.GROUP, expanded = false)
private String advanced = "Advanced options";

@Parameter(group = "basic", label = "Number of ducks")
private int duckCount = 1;

@Parameter(group = "basic", label = "Time of next feeding")
private Date nextFeeding = new Date();

@Parameter(group = "advanced", label = "Advanced duck typing")
private boolean advancedDuckTyping = true;

@Parameter(group = "advanced", label = "Starvation threshold")
private int starvationThreshold = 6;

If ParameterGroup is really better, I guess we could add methods for isParameterGroup and getParameterGroup in addition to CommandModuleItem etc.. But seems more awkward unless I miss the right way to do it.

ctrueden commented 3 years ago

Sure, I am OK with the new ItemVisibility scope. Thanks for working on this!

The new expanded boolean attribute of @Parameter is unfortunate (confusing for any normal parameters), but OK for now. With the struct-based redesign in SciJava 3, the Parameter annotation is being slimmed way down; hopefully we'll be able to come up with a better separation of concerns here for groups, as that work progresses.

ctrueden commented 3 years ago

Oh, also, I should mention: there is nothing stopping from stacking annotations on the same field. So you could conceivably do:

@Parameter(visibility = ItemVisibility.MESSAGE)
@ParameterGroup(expanded = false)
private String advanced = "Advanced options";

It's one more line, but would let you put the group-specific annotation parameters into their own annotation interface, while still using @Parameter itself for discovery.

karlduderstadt commented 3 years ago

Ah thanks for pointing that out. I didn't realize you can stack the annotations. That helps a lot if you also still have the normal Parameter annotation. I am still exploring the ParameterGroup strategy.

But will ParameterGroups work easily in scripts ?

Because I think putting everything in the normal Parameter annotation (using visibility = ItemVisibility.GROUP ) might more easily allow for this feature in scripts.

imagejan commented 3 years ago

But will ParameterGroups work easily in scripts ?

It would require adding a dedicated script directive, next to the (half-finished) #@script and #@import directives as well as #@initialize and #@callback, see these open issues.

But I'm not sure how it would be best combined with parameter annotations in scripts, so maybe auto-generating groups from the group=... attribute as a fallback is the best strategy.

karlduderstadt commented 3 years ago

@imagejan Thanks for clarifying, I didn't know about those open issues and all the possibilities in the pipeline for script parameters. OK for now I think I will do it all inside normal Parameters and include the ItemVisibility.GROUP as well as expanded annotation. This greatly simplifies what is required, I didn't make much progress last night with the ParameterGroups, just ended up with a lot of failed unit tests.

I really don't think it is strange or awkward to add expanded to Parameters. As I said above, there are many annotations that only apply to one type but they all live inside Parameter. Min and Max don't make a lot of sense for Strings etc..

This should allow me to finish quickly in scijava-common and jump back to scijava-ui-swing to try and implement everything inside a single InputPanel class.

ParameterGroups could always be added at a later time, once there is a working first draft.

karlduderstadt commented 3 years ago

I have a working implementation of widget groups. It relies on changes here to scijava-common, which are in the PR https://github.com/scijava/scijava-common/pull/428

With these changes, I implemented all the needed changes in SwingInputPanel of scijava-ui-swing, with no changes to other classes. These changes are in PR https://github.com/scijava/scijava-ui-swing/pull/60 the CI is broken for the PR because it depends on first merging of the PR to scijava-common and then having scijava-ui-swing depending on the new version.

The new format for adding groups is described in the PR comments. Parameters are added in the order provided, with the group label also showing up based on the order of parameters in java commands or scripts. I tested both (java commands and scripts) and both work. Any Parameters belonging to the group will be hidden or displayed based on the label status irrespective of order. This was the simplest way to implementing things.

@ctrueden where should I add the PrefService lines to ensure the expanded status can be persisted for consecutive runs of a command?

I could add it directly into the SwingInputPanel. That would actually be the easiest way, but perhaps there is a better place. However, the expanded status of group labels isn't currently resaved to anywhere accessible outside of SwingInputPanel.

karlduderstadt commented 3 years ago

Ok, I figured out how to have the expanded status persist using PrefService. It seems to require some small changes in SwingInputHarvester. That's where it has to live so the class is properly set equal to the module class.

But I have been exploring this a bit more and realized it might be cleaner to have a JTabbedPane. This could be an alternative group style option or perhaps replace the collapsible labels and wouldn't require the expanded status. The problem with the collapsible labels is that the dialog height needs to change frequently with the layout. I find it kind of awkward. With tabs the dialog size can usually stay unchanged.

Screenshot 2021-08-10 at 11 40 19
ctrueden commented 4 months ago

I did a bunch of work back in 2017 to redo the SciJava widget generation system, but my sole progress report is buried as a comment on another issue, which I always have trouble finding when searching... I usually end up back at this issue instead, which is very related. So I'm just adding a cross-link here now to make it easier to find:

https://github.com/scijava/scijava-common/issues/42#issuecomment-332658377