Refactor Sidepane logic#8202
Conversation
|
Great Idea, already looked into the issue about the view menu, but had not yet found a working solution. The enum of components seems to be a bit overcomplicated and inflexible too... |
- Add immutable info to SidePaneType enum - Replace SidePaneComponent by SidePaneView which has a SidePaneHeaderView that contains all input handling - Create SidePaneContainerView the replacement for SidePane
- Move some state and logic to SidePaneContainerViewModel - Implement the moveUp and moveDown action - Implement lazy loading for SidePaneView - Expose some initial API for fixing #8115
- Fix #8115 - Deprecate the old implementation of Sidepane and remove all of its usage in the rest of the application - Implement the toggle method in SidePaneContainerView
|
Codacy is complaining and saying that there is a missing enum branch in the switch statement which is not true, can someone tell me what's wrong? |
|
Forget about codacy! |
tobiasdiez
left a comment
There was a problem hiding this comment.
Thanks a lot for you work! This is sooooo much better already.
I have a couple of further suggestions mainly around the main direction of information flow (using the StateManager class) and using javafx bindings.
- Swapping elements in ObservableLists is rather difficult to handle. The problem with this code here is that two events are fired (one for removing, and on for adding). Perhaps the easiest solution is to use the sort method using a custom comparator. Since the list is small we can also be inefficient and do this as follows: create a copy of visiblePanes, swap the elements there and then use this new list as a reference for the comparator
|
Is this PR ready for final review now? |
|
Hi @HoussemNasri : since this PR was a bit stalled, I took the liberty to work a bit on it. After all the great work you already did, I just refactored SidePane and SidePaneViewModel a bit to better implement the MVVM-Pattern. Also I reduced the code duplication with the visible properties for the menu. Works with Bindings and only one source of truth now. The second thing I tried to work on was the bug about the not-closing sidepane, when every component is closed. This should work now (I moved the listener from the visible property to the children list of the sidepane), but the issue mentioned by @Siedlerchr (#8082 (comment)) still remains, which means that the initial width of the sidepane if open on startup is not right. Need to investigate some more. |
|
This should work now and close two bugs. |
|
Thank you @calixtus! I'm glad I got to work with you and other brilliant developers on this project, learning tons about making software according to best practices. I feel like I have more and better skills than ever now, too! I'll definitely be getting involved more as time goes on; both with Jabref and in the open-source community - so thank you again! |
|
@tobiasdiez Do you have any issues left? Otherwise I would like to merge this so we can get this in our next release asap |
tobiasdiez
left a comment
There was a problem hiding this comment.
I don't really have time to look at this in detail. If you think its fine, please go ahead and merge it.
* upstream/main: Don't register any database changes to the indexer while dropping a file (#8334) Fix ACM fetcher (#8338) Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5 Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247) Refactor Sidepane logic (#8202) New translations JabRef_en.properties (Japanese) (#8331) Bump bcprov-jdk15on from 1.69 to 1.70 (#8333) Update Controlsfx to 11.1.1 (#8330) Update citeproc (#8329) Bump classgraph from 4.8.137 to 4.8.138 (#8327) Bump junit-platform-launcher from 1.8.1 to 1.8.2 (#8326) Remove outdated gradle deps check (#8324) Bump junit-jupiter from 5.8.1 to 5.8.2 (#8325) Bump libreoffice from 7.2.2 to 7.2.3 (#8328) Remove outdated ignores
* upstream/main: (46 commits) New Crowdin updates (#8349) Bump pdfbox from 2.0.24 to 2.0.25 (#8345) Bump fontbox from 2.0.24 to 2.0.25 (#8348) Bump xmlunit-core from 2.8.3 to 2.8.4 (#8347) Bump tika-core from 2.1.0 to 2.2.0 (#8346) Added missing executable bindings to various commands (#8342) Update Gradle Wrapper from 7.3.1 to 7.3.2. (#8343) Fix issues with writing metadata to pdfs (#8332) add tinylog test (#8339) Tinylog (#8226) Don't register any database changes to the indexer while dropping a file (#8334) Fix ACM fetcher (#8338) Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5 Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247) Refactor Sidepane logic (#8202) New translations JabRef_en.properties (Japanese) (#8331) Bump bcprov-jdk15on from 1.69 to 1.70 (#8333) Update Controlsfx to 11.1.1 (#8330) Update citeproc (#8329) Bump classgraph from 4.8.137 to 4.8.138 (#8327) ... # Conflicts: # build.gradle
Refactoring goals:
Remove all view references fromSidePaneComponentInstead of having an instance ofSidePaneinSidePaneComponentfor updating the view, we should use bindingRemove unnecessary abstraction inSidePanelike (separating components and visibleComponents)Maybe refactorSidePaneComponentinto view and ViewModelEdit: I ended up redesigning the whole component so don't take the above goals as a grant
Added classes
SidePane, it has almost the same actions/methods (show, hide, toggle, moveUp, moveDown) but in private because they are not used from the outside, it has one public methodsidePaneVisibleProperty(sidePaneType)in the view, it returns the visibility property forsidePaneType, it's public because it's the only property used from the outside (check menu).SidePaneComponent#getHeader, this class controls the position/visibility of aSidePaneViewusing actions received in the constructor (moveUpCommand, moveDownCommand, closeCommand).SidePaneComponent, it glues together the content of the pane with the header.SidePaneViewwith 2 differences, anafterOpeningmethod and a special headerGroupsSidePaneHeaderViewbecause groups pane uses an extra button for toggling intersection union, I didn't putafterOpeningin the base implementation because the other sidepanes (webSearch and OpenOffice) don't use it.SidePaneHeaderViewwith an extra button for toggling intersection union, it handles it internally.WebSearchPane#createContentPaneinto its own class.Notes
visiblePaneslist changes inSidePaneContainerViewModel.afterOpeningandbeforeClosingmethods inSidePaneComponentbecause they are only adding and removing panes to/frompreferencesService.getSidePanePreferences().visiblePanes(), preferences visible panes are now updated fromSidePaneContainerViewModelinshow()andhide(), the only side pane that does something beside that, isGroupsSidePaneViewthat's why we addedafterOpeningto it.SidePaneTypehas the title, icon, and action of the side pane so it can be considered a model in the new design.CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)Desktop.2021.10.31.-.23.42.59.02.mp4
.