Skip to content
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

Split JabRefFrame to View and ViewModel #11180

Merged
merged 24 commits into from
Apr 16, 2024
Merged

Split JabRefFrame to View and ViewModel #11180

merged 24 commits into from
Apr 16, 2024

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Apr 10, 2024

Plan is to implement the mvvm pattern somehow in JabRefFrame and clean up a bit the gui root directory.

  • Split JabRefFrame into JabRefFrame and JabRefFrameViewModel
  • TabContainer means the tabbed pane, but it is part of JabRefFrame, so the wrapper methods (addTab close etc) stay there.
  • Moved JabRefFrame and the related classes to a distinct package frame
    tbc.

Please leave your comments and thoughts.

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@calixtus calixtus added the type: code-quality Issues related to code or architecture decisions label Apr 10, 2024
@koppor
Copy link
Member

koppor commented Apr 11, 2024

For viewing the diff, I recommend to try out RefactoringMiner. See https://github.com/tsantalis/RefactoringMiner/blob/master/docker/README.md for a howto.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far

src/main/java/org/jabref/gui/LibraryTabContainer.java Outdated Show resolved Hide resolved
@calixtus
Copy link
Member Author

Stopping refactoring here, since the pr has already grown very large and further refactorings require changes to LibraryTab and Globals, which would totally escalate this.

@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 13, 2024
@calixtus calixtus marked this pull request as ready for review April 13, 2024 15:04
Copy link
Contributor

github-actions bot commented Apr 15, 2024

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@Siedlerchr Siedlerchr enabled auto-merge April 16, 2024 19:00
@Siedlerchr Siedlerchr added this pull request to the merge queue Apr 16, 2024
Merged via the queue into main with commit a52d4b7 Apr 16, 2024
20 checks passed
@Siedlerchr Siedlerchr deleted the frameViewModel branch April 16, 2024 19:14
@LoayGhreeb
Copy link
Collaborator

@calixtus After this PR, many warnings related to CSS appear when trying to open the entry editor for the first time, but not for all libraries. For example, Chocolate.bib always reproduces these warnings, but jabref-authors.bib works without any warning.

WARN: Caught 'java.lang.ClassCastException: class java.lang.String cannot be cast to class javafx.scene.paint.Paint (java.lang.String is in module java.base of loader 'bootstrap'; javafx.scene.paint.Paint is in module javafx.graphics@22 of loader 'app')' while converting value for '-fx-background-color' from rule '*.editorPane' in stylesheet file:/C:/Users/Loay%20Ahmed/Documents/JabRef/build/resources/main/org/jabref/gui/entryeditor/entryeditor.css
2024-04-17 00:17:00 [JavaFX Application Thread] sun.util.logging.internal.LoggingProviderImpl$JULWrapper.log()
WARN: Could not resolve '-fx-text-background-color' while resolving lookups for '-fx-text-fill' from rule '*.label' in stylesheet jar:file:///C:/Users/Loay%20Ahmed/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-controls/22/3f2f311e6cff97b2571231e7d9d3acc6ac100d90/javafx-controls-22-win.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
2024-04-17 00:17:00 [JavaFX Application Thread] sun.util.logging.internal.LoggingProviderImpl$JULWrapper.log()
WARN: Could not resolve '-fx-text-background-color' while resolving lookups for '-fx-text-fill' from rule '*.label' in stylesheet jar:file:///C:/Users/Loay%20Ahmed/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-controls/22/3f2f311e6cff97b2571231e7d9d3acc6ac100d90/javafx-controls-22-win.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss
2024-04-17 00:17:00 [JavaFX Application Thread] sun.util.logging.internal.LoggingProviderImpl$JULWrapper.log()


And more...

I looked into this issue, and the changes were made in the pull request. I tried to revert the commit 8efbaad (#11180), and it works without any warnings.

@calixtus
Copy link
Member Author

calixtus commented Apr 17, 2024

I see, thing is, the controlsfx decoration pane is made for some completely different purpose (showing another node on top of a label for validation). So the warnings are showing that there was something wrong before I removed the decoration pane. I will look into it.

@calixtus
Copy link
Member Author

I tried, but I cannot reproduce...
Did you try gradle clean?

@LoayGhreeb
Copy link
Collaborator

Did you try gradle clean?

tried now, but still able to reproduce

@calixtus
Copy link
Member Author

Similar here: mkpaz/atlantafx#65 (comment)

@koppor
Copy link
Member

koppor commented Apr 17, 2024

I searched our issue tracker and found #8628. I think, however, the exception is not risen any more and that issue can be closed - and we need a new issue to track?

@LoayGhreeb
Copy link
Collaborator

In the latest release (5.13), there are no warnings when opening the entry editor. However, upon opening the "Search results" tab, these warnings appear in the event log:

Caught 'java.lang.ClassCastException: class java.lang.String cannot be cast to class javafx.scene.paint.Paint (java.lang.String is in module java.base of loader 'bootstrap'; javafx.scene.paint.Paint is in module javafx.graphics@22 of loader 'app')' while converting value for '-fx-background-color' from rule '*.progress-indicator:indeterminate *.segment' in stylesheet jar:file:///C:/Users/Loay%20Ahmed/.gradle/caches/modules-2/files-2.1/org.openjfx/javafx-controls/22/3f2f311e6cff97b2571231e7d9d3acc6ac100d90/javafx-controls-22-win.jar!/com/sun/javafx/scene/control/skin/modena/modena.bss

The exceptions mentioned in issue #8628 are not raised.

In the latest main, the same behavior occurs along with the warnings when opening the entry editor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants