-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Sidepane logic #8202
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/main/java/org/jabref/gui/sidepane/GroupsSidePaneHeaderView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/GroupsSidePaneHeaderView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerViewModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerViewModel.java
Outdated
Show resolved
Hide resolved
- 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
src/main/java/org/jabref/gui/sidepane/SidePaneContainerView.java
Outdated
Show resolved
Hide resolved
Is this PR ready for final review now? |
src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneView.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/sidepane/SidePaneContainerViewModel.java
Outdated
Show resolved
Hide resolved
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromSidePaneComponent
Instead of having an instance ofSidePane
inSidePaneComponent
for updating the view, we should use bindingRemove unnecessary abstraction inSidePane
like (separating components and visibleComponents)Maybe refactorSidePaneComponent
into 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 aSidePaneView
using actions received in the constructor (moveUpCommand, moveDownCommand, closeCommand).SidePaneComponent
, it glues together the content of the pane with the header.SidePaneView
with 2 differences, anafterOpening
method and a special headerGroupsSidePaneHeaderView
because groups pane uses an extra button for toggling intersection union, I didn't putafterOpening
in the base implementation because the other sidepanes (webSearch and OpenOffice) don't use it.SidePaneHeaderView
with an extra button for toggling intersection union, it handles it internally.WebSearchPane#createContentPane
into its own class.Notes
visiblePanes
list changes inSidePaneContainerViewModel
.afterOpening
andbeforeClosing
methods inSidePaneComponent
because they are only adding and removing panes to/frompreferencesService.getSidePanePreferences().visiblePanes()
, preferences visible panes are now updated fromSidePaneContainerViewModel
inshow()
andhide()
, the only side pane that does something beside that, isGroupsSidePaneView
that's why we addedafterOpening
to it.SidePaneType
has the title, icon, and action of the side pane so it can be considered a model in the new design.CHANGELOG.md
described in a way that is understandable for the average user (if applicable)Desktop.2021.10.31.-.23.42.59.02.mp4
.