search

scijava / ui-behaviour

Configurable input handling, via mapped behaviours
Other
4 stars 2 forks source link

VisualEditorPanel: ill-named class of Listeners #23

Closed xulman closed 3 years ago

xulman commented 4 years ago

Hi there,

after the PR #20, users of the Panel can now attach to two sorts of listeners. See the example code below:

        final InputTriggerConfig config = new InputTriggerConfig();
        final VisualEditorPanel panel = new VisualEditorPanel( config );
        panel.configChangeListeners().add( () -> System.out.println( "GUI Item changed" ) );
        panel.configCommittedListeners().add( () -> System.out.println( "GUI's Apply button was pressed" ) );

What happens? On line 3, the underlying config is not yet altered, only panel's internal data was updated. On line 4, the underlying config is updated, and panel's data hasn't changed. But on line 3, the listener's name tell different story. Line 4 is IMHO OK.

There's also a discrepancy in the JavaDocs now: https://github.com/scijava/ui-behaviour/blob/b14b7786587cbd1cbbca734a07483a29ce004e47/src/main/java/org/scijava/ui/behaviour/io/gui/VisualEditorPanel.java#L156-L161

Good thing is that the original add-listeners method is now deprecated: https://github.com/scijava/ui-behaviour/blob/b14b7786587cbd1cbbca734a07483a29ce004e47/src/main/java/org/scijava/ui/behaviour/io/gui/VisualEditorPanel.java#L854-L858

So, before the new stuff is inaugurated in the main pom-scijava (https://github.com/scijava/pom-scijava/pull/138), I propose to fix this up:

I volunteer to prepare a PR but wanted to discuss it with you first... and agree on the names, etc. (I will even try to adhere to the coding std. much closer than last time, I promise) Vlado

tpietzsch commented 4 years ago

I'm fine with renaming configChangeListeners to something better and also with introducing a new interface for that. About the "two new params: behaviour string ID and (the new) key trigger", I'm not so sure. That event is also fired in other circumstances, for example when a binding is removed or when contexts are changed. I'm not sure about the details, but possibly the change triggering one event is not always limited to one behaviour. So in short: I would rather stay at that "something has changed (you figure it out)" semantic. Then itemChanged is not the best name. Maybe modelChanged... would be a better name, fitting with the rest of the naming in the class (e.g., there is configToModel and modelToConfig methods).

xulman commented 4 years ago

hi @tpietzsch, I've added (hopefully) a solution to this issue, please find it in the PR #24

xulman commented 4 years ago

reminder of the relevant: https://github.com/scijava/pom-scijava/pull/138

xulman commented 3 years ago

solved with https://github.com/scijava/ui-behaviour/pull/24