-
-
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
Fix #12272 - Show a welcome screen if no database is open #12461
base: main
Are you sure you want to change the base?
Conversation
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.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
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.
Just a quick review from my cellphone. A more deeper look will follow.
-fx-font-family: "Arial"; | ||
} | ||
|
||
.description-label { |
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.
Please use a more explicit selector name for the CSS class, as this may interfere with other places similar or the same classes are used. Use welcome-* like above and ahead as a prefix.
/* AboutDialog */ | ||
#aboutDialog .about-heading { | ||
-fx-font-size: 30; | ||
-fx-font-size: 31; |
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.
Why this change?
@@ -230,11 +258,27 @@ private void initLayout() { | |||
setTop(head); | |||
|
|||
splitPane.getItems().addAll(tabbedPane); | |||
contentPane.getChildren().addAll(welcomePage, splitPane); |
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 would suggest to implement this as a separate tab and not as a static background. Take vscode as an example.
This way, the welcome page can be kept shown and revisited later if the user wants to work on a library in parallel. I think implementation would be straightforward as a new tab.
This would also take a lot of complexity out of the node tree.
|
||
private void updateContent() { | ||
boolean hasOpenDatabases = !stateManager.getOpenDatabases().isEmpty(); | ||
welcomePage.setVisible(!hasOpenDatabases); |
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.
If the welcome page is the most 'lowest' node in a StackPane, it should automatically be hidden if a library is shown above. This is the whole purpose of a StackPane to deal with this. So there should be no need for messing with visibility stuff here...
import org.jabref.model.entry.BibEntryTypesManager; | ||
import org.jabref.model.util.FileUpdateMonitor; | ||
|
||
public class WelcomePage extends VBox { |
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.
Extends Tab?
welcomeLabel.getStyleClass().add("welcome-label"); | ||
|
||
Label descriptionLabel = new Label(Localization.lang("Stay on top of your Literature")); | ||
descriptionLabel.getStyleClass().add("description-label"); |
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.
Explicit class names
return createBox(Pos.TOP_LEFT, welcomeLabel, descriptionLabel); | ||
} | ||
|
||
private VBox createStartBox(JabRefFrame frame, GuiPreferences preferences, AiService aiService, DialogService dialogService, StateManager stateManager, FileUpdateMonitor fileUpdateMonitor, BibEntryTypesManager entryTypesManager, CountingUndoManager undoManager, ClipBoardManager clipBoardManager, TaskExecutor taskExecutor) { |
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.
Why not class variables? I don't see any need for method injection.
} | ||
|
||
private VBox createBox(Pos alignment, Node... nodes) { | ||
VBox box = new VBox(5); |
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.
Why fixed count for initialization?
} | ||
} | ||
|
||
private VBox createBox(Pos alignment, Node... nodes) { |
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.
CreateAlignedVBox?
Hi @calixtus! Saw all the comments with respect to the PR. Will resolve them asap. Thanks for the review! |
Describe the changes you have made here: what, why, ...
"Closes #12272"
"Closes #12272".
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)Screen.Recording.2025-02-04.at.3.49.55.PM.mov