T23749: Initial commit of the data storage viewer base class
Description
Description
Details
Details
- Auditors
floca kalali - Provenance
kalali Authored on Dec 4 2017, 2:37 PM kalali Pushed on Dec 4 2017, 2:38 PM - Differential Revision
- Restricted Differential Revision
- Parents
- rMITK0b7158a90336: Merge branch 'T23268-more-clang-fixes'
- Branches
- Unknown
- Tags
Event Timeline
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h | ||
---|---|---|
36–53 | We have to talk about the interplay between this class and the provided view and QmitkIDataStorageViewModel. | |
65 | Getter? | |
71 | Getter? | |
83 |
| |
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 |
| |
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). |
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h | ||
---|---|---|
36–53 | My bad, this should be called something like QmitkDataStorageAbstractViewer. 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? | |
196–201 | This is what I did (workaround) in my default list model (by overriding) in T23752. 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). |
/Modules/QtWidgets/include/QmitkDataStorageAbstractView.h | ||
---|---|---|
36–53 | And QmitkIDataStorageViewModel could also better be called QmitkIDataStorageModel |
/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp | ||
---|---|---|
126–136 | Korrekt, habe vergessen die Vektoren zu sortieren. |
/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). |
Comment Actions
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. |
/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. |
/Modules/QtWidgets/src/QmitkDataStorageAbstractView.cpp | ||
---|---|---|
142–146 |
Comment Actions
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.