bigdataviewer / bigdataviewer-core

ImgLib2-based viewer for registered SPIM stacks and more
BSD 2-Clause "Simplified" License
33 stars 35 forks source link

SourceAndConverter and Source are org.scijava.Named objects. #89

Closed NicoKiaru closed 2 years ago

NicoKiaru commented 4 years ago

This means the names are not final anymore. Is this acceptable ?

By default SourceAndConverter objects take the name of the associated SpimSource given in the constructor, or duplicate the name if the constructor called is the one which needs a SourceAndConverter as an input. This initial SourceAndConverter name can be changed by using the method SourceAndConverter::setName.

There is a default setName method which throws an exception in the Source interface. This avoids creating a setName method for all the other projects which depend on bigdataviewer-core. But is this a good idea ? Removing the default setName would force to have a correct implementation for Source subclasses, instead of throwing an exception.

As source names are not final, there's a risk to loose synchronization with the display in the bigdataviewer UI.

tpietzsch commented 4 years ago

As source names are not final, there's a risk to loose synchronization with the display in the bigdataviewer UI.

Yes, this is an important point. If possible, it would be best to make source name setting always go through the ViewerState. For example, there is already ViewerState.setGroupName(SourceGroup, String) that will emit a ViewerStateChange event GROUP_NAME_CHANGED, is subject to locking/synchronization etc. It would be desirable to have the same for the source names. E.g., if you synchronize on the ViewerState, you can be sure that no-one will change source names under you.

This then leads to the question whether the name should even be a property of the Source or stored directly in the ViewerState (as for groups). The downside is then that you cannot modify the name of a Source without it being attached to a ViewerState. And the same source shown in different BigDataViewer windows could be given different names (which is good or bad, depending on what you want).

In any case it is important that changing the name must not change the hashcode of the SourceAndConverter (and therefore Source). because SourceAndConverter is the key of the source in the ViewerState.

NicoKiaru commented 4 years ago

Ok, that's indeed much more complicated than I thought....

Let's suppose an identical sourceandconverter is present in multiple bdv windows. An orthoviewer could be like that for instance.

If the name of Source is a property stored in the ViewerState:

I do not see how a single name can be maintained in sync with multiple bdv windows without replicating a mechanism of listeners like the one present with ConverterSetup ( changing min max / color of ConverterSetup works well in sync with multiple bdv windows ).

So I see three options:

I do not see any other solution that would work for multiple bdv windows, and which still allows renaming.

Now if we do not allow renaming and keep the name final. That will be much less work. What could still be super useful is to add a name field in SourceAndConverter, which would copy the name of the SpimSource by default, and then add SourceAndConverter constructors with a name parameter which would would allow to override this behaviour. This would allow to have unique names for SourceAndConverter. But the uniqueness has to be done on the creation of the SourceAndConverter object. Then the setName method would throw an unsupported operation exception.

Side notes:

In any case it is important that changing the name must not change the hashcode of the SourceAndConverter (and therefore Source). because SourceAndConverter is the key of the source in the ViewerState.

Do you override the hashCode or equals method of SourceAndConverter somewhere ? From what I understand in [hashcode javadoc](https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#hashCode()), there' s no risk this should happen.


Regarding usage scenario, it's not super straightforward, but here's my use case:

https://youtu.be/tuiaEXmFVyE?t=20

https://user-images.githubusercontent.com/20223054/80470278-11efda80-8942-11ea-86c6-a232eb728e80.png

As soon as I have multiple files, I usually end up with duplicate names in Source or SourceAndConverter. And this prevent me from using macro scripting for instance, because identifying a source is ambiguous. So that's an issue.

Also when using the scijava framework, if an object do not override toString or is not a Named object, then it usually have an ugly name in widgets. And because bdv-playground is internally using SourceAndConverter objects, it currently have ugly name.

Ok, maybe not everything is useful here, but the issues we are facing with @tischi in the bdv-playground repo are :