HomePhabricator
Diffusion MITK dab8aa4b04e1

T23749: Initial commit of the data storage viewer base class

Authored by kalali on Dec 4 2017, 2:37 PM.

Description

T23749: Initial commit of the data storage viewer base class

Details

Auditors
floca
kalali
Committed
kalaliDec 4 2017, 2:37 PM
Pushed
kalaliDec 4 2017, 2:38 PM
Differential Revision
Restricted Differential Revision
Parents
rMITK0b7158a90336: Merge branch 'T23268-more-clang-fixes'
Branches
Unknown
Tags
Unknown

Event Timeline

floca raised a concern with this commit.Dec 20 2017, 9:57 AM
floca added a subscriber: floca.
floca added inline comments.
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h
36–53

We have to talk about the interplay between this class and the provided view and QmitkIDataStorageViewModel.
Either the naming of QmitkDataStorageAbstractView is very missleading or the interplay between the three is very convoluted and at least to me unclear.

65

Getter?

71

Getter?

83
  1. This should be a slot.
  2. Where is the getter?
  3. I would change the name of the method and the variable to better resemble the job. E.g. RepresentOnlyVisibleNodes or ModifyOnlyVisibleNodes. Because at the end it is about the class either representing/manipulating the whole provided selection or only the part not "masked" by the filter
91

Please over also the option to use mitk::DataStorage::SetOfObjects. This would be the natural type provided from the storage when it comes to selections of nodes.

106

Please over also the option to use mitk::DataStorage::SetOfObjects. This would be the natural type provided from the storage when it comes to selections of nodes.

120

Naming sounds more like a signal. ChangeModelSelection?

155–161

Seems illgical to keep these functions private but the members protected. Had Assumed the other way arround or both protected.

/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp
61–64

Why not allowing the storage to be set bakc to a nullptr again?

126–136

Das ist kein echtes equal sondern nur ein ordered equal.

Ich würde diesen Codeblock in eine eigene private Hilfsfunktion auslagern. Die Methode wird sonst zu unübersichtlich und du kannst dir den Kommentarsparen, da du einen sprechenden Methodennamen hast.

191–204
  1. I would expect that at least and always the update of the model/view is triggered, because the storage state is changed.
  2. This should be always done. So you can but the "always" part in own methods that just call this virtual functions that are allowed to override.
196–201

I would expect that these methods should actualized the current selection. Either it could be that the node is not part of the storage any more (NodeRemoved) or that its filtered out noe (NodeChanged).

This commit now has outstanding concerns.Dec 20 2017, 9:57 AM
kalali marked an inline comment as done.Dec 20 2017, 12:20 PM
kalali added inline comments.
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h
36–53

My bad, this should be called something like QmitkDataStorageAbstractViewer.
However, I'm open to something else than "viewer", as this is to close to the Qt-view and the berry-biew.

Actually this class is not a viewer, but holds a Qt-view (and the underlying model). This class is mainly for connecting the view/model with the data storage. So it could be named something like QmitkAbstractDataStorageSelectionConnector.

I named the class in T23752 'QmitkDataStorageSelectionConnector'. However, this class could be named something like 'QmitkSelectionServiceConnector' or the like.

But anyways, there is another idea how to use this class, so the naming might be totally different. Will elaborate on that in the referenced task.

120

Agreed.

/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp
61–64

Should be allowed. I already changed this in 03f08793da58.

191–204

Maybe I don't get this, but what is the difference between "trigger the update of the model/view" and then calling e.g. NodeAdded, which might again trigger some kind of update of the model/view?
Shouldn't it be enough to just propagate the event to something like m_Model->NodeAdded?

196–201

This is what I did (workaround) in my default list model (by overriding) in T23752.
However, I did not want to force any behavior with this event.

Again, elaborating on a different use of this class in the referenced task, these functions might only be used to propagate the event to the model (m_Model->NodeRemoved).
Then we could force to implement the NodeRemoved-function by extending the QmitkIDataStorageViewModel.

kalali marked 2 inline comments as done.Dec 20 2017, 12:24 PM
kalali added inline comments.
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h
36–53

And QmitkIDataStorageViewModel could also better be called QmitkIDataStorageModel

kalali added inline comments.Dec 20 2017, 1:31 PM
/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp
126–136

Korrekt, habe vergessen die Vektoren zu sortieren.
Habe es ausgelagert und mit std::is_permutation gelöst.

kalali marked 2 inline comments as done.Dec 21 2017, 12:45 PM
kalali added inline comments.
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h
91

This is purely qt specific and only converts the selected qt indices into the underlying items, which happen to be data nodes. I see your point, but I'm wondering if it is ok, to just use the SetOfObjects in those two cases, where the nodes come from outside or are sent outwards (e.g. not changing the return type of ´GetSelectedNodes` or FilterNodeList).

goch added a subscriber: goch.EditedDec 22 2017, 2:21 PM

I agree that the viewer has too many signals and selection capability for my understanding of a viewer.

/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp
142–146

I do not understand the why behind this.

kalali added inline comments.Dec 22 2017, 3:40 PM
/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp
142–146

The idea is, that a view may only be able to show nodes a,b,c,d, whereas another view is able to show a,b,c,d, e,f.
So if the first view receives the signal to select nodes c,d,e, should it throw away the selection of node e? If so, then there is nothing else to do.
If not, the we have to store the selection of node e, since it will not be send from the first view (because it is not shown and therefore can not be de-/selected). We need to include the selection of node e later, to send it to everyone, since it is a remainder from the first selection.

kalali added inline comments.Jan 2 2018, 10:59 AM
/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp
142–146

@floca : need to talk about this again.
Comment from @goch that it should not be allowed to store "not-visible" data nodes in the background. This could totally confuse the user, especially if more than two dataStorageViewers are used as selection provider.

floca added a comment.Jan 2 2018, 1:48 PM

I agree that the viewer has to many signals and selection capability for my understanding of a viewer.

From my point of view, the selection capability is plausible and should not be removed. @kalali, I would keep it, as discussed lately. Especially in the light of our last discussion and the role we shaped out. @goch, the reason in the current code state is a bit cluttered by the naming/role mash up. But with the roles/responsibilities Amir has clarified, I think it is a sensible feature.
The role of the class is to establish sync between the outside world/selection (e.g. specified business logic or selection bus) and the view on a data storage (and the selection model modified by the user in a widget), that may be filter by predicates. In this situation it is usefull to have, as a developer, the posiility to decide how the sync mechanism should handle selection the user cannot even see in the view because they do not match the predicate.

The "confuse the user" argument does not hold generaly. You could think of use cases where it confuses they user equally, if the "hiden" selection gets lost. Then she/he would wonder, "why is the selection gone? I haven't seen/altered them at all, in my last selection operations, because I only changed image and segmentation selections. But now my registration selections went *puff* without a warning.".
More abstractly spoken, one would not be able to allow predicate-specific sub selection modifications of a whole selection set in a plugin, if we skip the feature in the sync-class.

So bottom line. keep the feature in the sync-class. There it makes sens and it is of value. Don't have something like that in the StorageModelClass and so on. There it makes no sense and would do more evil than good.

Hope it gets clearer now.

kalali marked 5 inline comments as done.Jan 3 2018, 6:09 PM
kalali accepted this commit.Mar 23 2018, 5:37 PM
kalali marked 14 inline comments as done.
floca accepted this commit.Apr 3 2018, 2:11 PM
All concerns with this commit have now been addressed.Apr 3 2018, 2:11 PM