Closed NicoKiaru closed 4 years ago
love it, do it!
Cool!
One thing where I'm not completely sure is where to put the Commands:
Here are several options:
What do you think @tischi and @haesleinhuepf ?
I agree that the granularity of Command
s could well be different from actions. Thus it is maybe better to put them into an own command
package at the same level as bdv
and source
.
good, in this case I'll start the refactoring, is that ok ?
ok, I start
Ok done @tischi and @haesleinhuepf , let me know what you think.
Starting SimpleIJLaunch
enables FIJI and you can try Command from the Bdv_Scijava
root menu (should work)
I think all commands work but maybe we should add imagej-legacy as a dependency to enable old school FIJI
final public static String RootMenu = "Bdv_Scijava>";
to simply final public static String RootMenu = "BigDataViewer>";
, because I think end users would not care about SciJava
, sounds too technical to me.public class ActiveBdvPreprocessor extends SingleInputPreprocessor<BdvHandle>
would prefill the bdvHandle
parameter such that it would not show up? This is great!Command
s already appear in the main Fiji UI, I guess it would not make sense to add them again to the Bdv
window, right? We should discuss this with Tobias, because it might be confusing for users that there are now two menus that act on their currently open Bdv Window. On the other hand, if you have multiple Bdv
open it might be not so obvious, which BdvHandle
is the right one. Thus maybe actually is better to have the menus directly in the Bdv window rather than in the main Fiji window? Unless there are operations where you would need to select multiple bdvHandle
?!Hey guys,
great idea for the refactoring @NicoKiaru !
Regarding the separate command
package at the level of sources and bdv. I vote against that. So far, we had a ui
package inside specific packages, where Commands
were supposed to live. My major concern is: If we have a structure like:
bdv
package...
actionA
actionB
commands
package...
actionAcommand
actionBcommand
... it may become complicated to keep packages above actionA and actionAcommand in sync. Or: Somebody doesn't even know that there is a command for actionA and programs one. Thus, I vote for keeping actions and corresponding actionCommands close by each other. E.g. we could have ui
packages in every package to keep user-interface related stuff (such as commands).
bdv
package...
ui
actionAcommand
actionBcommand
actionA
actionB
Cheers, Robert
hi @haesleinhuepf,
I initially thought so, too, but @NicoKiaru and I thought that some actions may be too granular to be worth being exposed by a Command
and Command
s would quite often wrap several actions to be useful. See the discussion here: https://github.com/haesleinhuepf/bigdataviewer-playground/issues/21
final public static String RootMenu = "Bdv_Scijava>";
to simplyfinal public static String RootMenu = "BigDataViewer>"
Sure, the idea was just to have it at a root level because it's more convenient to test. When used for real, we will need to change to a proper location. The point of putting it here is that we will need to change only one value and not all menuPath
of all commands.
Am I right that this public class ActiveBdvPreprocessor extends SingleInputPreprocessor
would prefill the bdvHandle parameter such that it would not show up? This is great!
Yes that comes from @imagejan. Also there's a trick mechanism which should makes the last clicked bdvwindow to be the active one. A bit hacky but really convenient for user. One issue (very general), is that the parameter is not recorded when fed like that.
A general question: If those Commands already appear in the main Fiji UI, I guess it would not make sense to add them again to the Bdv window, right?
Up to you, up to us. ImageJ people use the menu, macro, and also shortcuts. The main cool idea idea is to trigger the same piece of code. So why so reuse in Menu / Behaviour ?
On the other hand, if you have multiple Bdv open it might be not so obvious, which BdvHandle is the right one.
Just FYI, there's a mechanism to select the one which is in focus currently.
Thus maybe actually is better to have the menus directly in the Bdv window rather than in the main Fiji window?
Why not both ? Genuine question.
Unless there are operations where you would need to select multiple bdvHandle?!
Typical case : synchronizing views on Bdv windows. Just FYI again, if Command required two Bdv windows, it will ask explicitely for which windows you want to synchronize.
Regarding the separate
command
package at the level of sources and bdv. I vote against that.
Yep we discussed that here: https://github.com/haesleinhuepf/bigdataviewer-playground/issues/32#issuecomment-565959452
And here: https://github.com/haesleinhuepf/bigdataviewer-playground/issues/21
fetch source
should be in source.importer
put in bdv
is in bdv.source.append
set color
and adjust contrast
are in bdv.source.displayopts
If we allow Commands
to be less granular, where should we put it/them ?
Happy to hear your opinion. Personnally I like the granularity but I also understand that too many commands is a mess for the user. Where to put Commands
if they are not as granular as actions ?
@haesleinhuepf
I also thought : if you want a ui
completely different from the Scijava one, wouldn't it be good to have it well separated ?
So, I was working in projects where we have a stucture like the following and enjoyed it a lot. Also, one could later split the project into UI and non-UI repositories and everything would still live in the same package. That would actually allow to have package-private access from the UI to parts of the algorithms which are not supposed to be accessible from the outside. The benefit comes for users when they explore a specific functionality. If they are interested in how to manipulate a Source, they find algorithms and corresponding user interface in the same place.
project
algorithm1
ui
swing
javafx
datastructures
operations
io
algorithm2
The principle behind is "What belongs together should stay together". Here is a nice article to read about that principle: https://medium.com/clarityhub/low-coupling-high-cohesion-3610e35ac4a6
If classes know each other, they should live in the same package or module. If they don't know each other, they should be separated. I thought separating actions on sources and actions on BDVHandles makes sense. But I wouldn't separate them from their respective user interface. BTW I'm not insisting in that structure, just suggesting. It does have pros and cons ;-)
Let's try with source.ui
, bdv.ui
, spimdata.ui
and see how it goes ? But not ui everywhere ?
What about we try putting ui
packages following the principle "What belongs together should stay together" in a sense that each ui
package (i) should not access major things in packages that are above it (maybe except some very general utilities methods) and (ii) should not have packages beneath it where it does not access any functionality.
Raah a part of my previous comment was lost. Let's take a precise example and see what we all think.
So let's say you want of Command which takes a File and display it into a BdvHandle (do you ? this makes sense no ? ). This consists of three actions : fetching sources (fetch source
), append sources on a BdvHandle (append source
) , adjust display options for the source within the Bdv (set color
, adjust contrast
)
fetch source
should be in source.importer
append source
is in bdv.source.append
set color
and adjust contrast
are in bdv.source.displayopts
Where would you put this command @tischer and @haesleinhuepf ? What's your personal opinion ?
Personnally I would put it eiher in a separate command package or in bdv.ui
. But it uses actions in source.importer
and bdv.source
.
Imho, this would have to be in a separate package at the same level as bdv
and source
, because it uses functionality from within source
and from within bdv
.
Just to make sure, there is no way to do this right. Be we should be consistent in doing it.
As bdv
uses sources
, sources
could be a sub-package of bdv
and so could be any ui
-related thing. On the other hand, bdv
is a viewer for sources
and thus, bdv
could be a sub-package of sources
. Likewise ui
-related stuff. How about sticking to Tobias structure? That would mean something like
bdv
sources
ui
Well, you have more experience than us + the Tobias authority argument ;-) Ok you win.
I think one reason why this is causing an issue is because the actions on source are well defined, but there is no ui which act on Source only. We always act on Sources which are either inside a bdv viewer or inside a spimdata, or maybe inside something like sciview.
So : all the ui will mostly look like :
Right now "somewhere" is very often a bdv viewer. But this could change in the future. Maybe we will have a central Source
manager, where each Source
is somehow linked to where it is displayed and to its metadata.
I can see the advantage of having the user interface linked in the repo to its action implementation. If one day the logic of the ui do not rely on BdvHandle, we will know where to look to implement the new logic.
So I'm ok to revert the ui location. I can do it on a separate branch and we'll continue the discussion ? Or do you want to do it yourself @haesleinhuepf ?
We discussed this in a skype meeting some time ago.
I'm closing it because I think a lot of changes have occured. If we need another discussion, then maybe we open a new issue with specific points ?
Please reopen if you do not agree!
Hi @tischi and @haesleinhuepf,
I have a suggestion to make about the structure of the repo. What do you think of restructuring like this:
bdv
: A BdvHandle specific package containing all actions that strictly depend on aBdvHandle
:bdv.navigate
: Loging, getting current position, synchronizing position ofBdvHandle
(s)bdv.group
: All actions related to Source groupingbdv.source
: actions on Sources which depends on the presence of aBdvHandle
bdv.source.screenshot
: Screenshot actionbdv.source.displayopts
: source display options : (B&C, hide, show, set Color)bdv.source.append
: Action which adds Sources toBdvHandle
(append a singleSource
, or aList<Source>
, or aSpimData
, or aSourceAndConverter
etc)bdv.source.get
: action which outputsSource
s in various ways from aBdvHandle
(principally from source indexes for instance) orSourceAndConverter
source
: Actions on Sources, as much independent ofBdvHandle
as possiblesource.transform
: Affine transformation / Warping transformation, Resampling etcsource.register
: here would go automated registrations and BigWarp (BigWarp is BdvHandle dependent however)source.samples
: get sample Sourcessource.import
: importList<Sources>
from Xml/Hdf5 , or from a tiff, or from IJ1 with the active window...source.export
: export ListAs discussed with @tischer I would need to use
Spimdata
so a specificSpimdata
package:spimdata
spimdata.import
getSpimData
from Xml/Hdf5 or from BigdataServerspimdata.export
basically save an open spimdata object