search

FelixBaensch / MORTAR

MOlecule fRagmenTAtion fRamework
MIT License
18 stars 3 forks source link

Unifying view tools, histogram and overview, to some extend and persisting their settings within and between sessions #6

Closed JonasSchaub closed 1 year ago

JonasSchaub commented 1 year ago

Dear @FelixBaensch , @BetuelSevindik , and @SamuelBehr ,

I have unified the histogram and overview "view tools" now to some extend. They are managed now through the new "ViewToolsManager" class which is used to open the views and -most importantly- takes care of their settings, so that they are persisted within a MORTAR session and also between MORTAR sessions.

Please, review and test as much as you can!

Also, some of the open TODO comments I just won't fix now, so that I stop blocking the repository for now.

JonasSchaub commented 1 year ago

About the merge conflicts: ListUtil has been renamed to CollectionUtil and extended on master but I have made only small changes here in that class; so that should be easy to amend. The only conflicting changes in HistogramViewController are also referring to this: The usage of ListUtil has -of course- been changed to CollectionUtil on master, but that's it. So, also easy to fix.

JonasSchaub commented 1 year ago

Fixed the merge conflicts 👍

SamuelBehr commented 1 year ago

Dear all,

after reviewing the code related to the OverviewView, I am very pleased with the changes you have done, @JonasSchaub. The ViewToolsManager is a really nice addition to the code. Also I do not find the functionalities of the overview view to have changed in any kind of way due to your changes - except from the grid configuration being persisted, which seems to be working fine.

One thing I noticed: Because the configuration of the grid of the overview view is now persisted - but not the size of the window, it is now possible that the overview opens and the persisted number of structures cannot be displayed. However, since I assume that a user who has set such a high number of rows and columns in a previous use of the overview view will adjust the size of the overview window afterwards anyway, this should not be a problem. This situation is handled as it can be seen in the attached picture. Screenshot 2023-06-19 151259

JonasSchaub commented 1 year ago

@SamuelBehr if the user then enlarges the window, the structures appear at some point, right? If yes, I would declare this as a "won't fix" because no window size in MORTAR is cached or persisted at this point.

SamuelBehr commented 1 year ago

@JonasSchaub Yes, exactly. The structures are visible again at the latest when the privious window size is reached.

Since this might be "a barrel we should not open" and since it is not really a problem anyways, I am fine with declaring it as "won't fix".

JonasSchaub commented 1 year ago

@SamuelBehr, more like a "box" of a specific character from Greek mythology.

I am fine with declaring it as "won't fix".

Great, then please approve the pull request.

JonasSchaub commented 1 year ago

@FelixBaensch and @B-s123 please (re-)confirm that this can be merged now.

B-s123 commented 1 year ago

approved