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

Show a welcome screen if no database is open #12461

Merged
merged 70 commits into from
Mar 19, 2025

Conversation

oops-shlok
Copy link
Contributor

@oops-shlok oops-shlok commented Feb 4, 2025

Closes #12272

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • 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.
Screen.Recording.2025-02-04.at.3.49.55.PM.mov
Screenshot 2025-02-04 at 3 49 48 PM Screenshot 2025-02-04 at 3 48 51 PM

Copy link
Contributor

@github-actions github-actions bot left a 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".

Copy link
Contributor

@github-actions github-actions bot left a 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.

Copy link
Member

@calixtus calixtus left a 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.

@oops-shlok
Copy link
Contributor Author

Hi @calixtus! Saw all the comments with respect to the PR. Will resolve them asap. Thanks for the review!

@subhramit subhramit changed the title Fix #12272 - Show a welcome screen if no database is open Show a welcome screen if no database is open Feb 5, 2025
Copy link
Contributor

@github-actions github-actions bot left a 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 Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

@oops-shlok oops-shlok requested a review from calixtus February 5, 2025 20:21
Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Hey @oops-shlok, thanks for your contribution!
Please revert the submodule changes (steps linked below).

Also, please don't resolve conversations from your end until the reviewer who asked for the change is satisfied and does that - otherwise each comment has to be manually expanded when trying to gather context on the changes asked (when re-reviewing) or to read your replies and carry the discussion forward.
For very trivial comments (for example, these ones below), you can mark resolved once done.

Copy link
Member

Choose a reason for hiding this comment

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

Undo the changes to submodules. Follow the alternative method in https://devdocs.jabref.org/code-howtos/faq.html#submodules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Will do this from now. Reverted the changes to submodules.

Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the changes to submodules

@oops-shlok oops-shlok requested a review from subhramit February 5, 2025 22:09
})),
Duration.seconds(5));
} else {
LOGGER.warn("Library Tab is null as Welcome Tab is open");
Copy link
Member

Choose a reason for hiding this comment

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

This is not a warning.

Please store tabSupplier.get() in a variable. You can use the refacroing functionality of IntelliJ for that ("extract variable)

Just add a comment above the if to say that the tab is null in case of the welcome page.

Comment on lines 80 to 82
if (libraryTab == null) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

This reads like a code smell...

The action has to be disabled if no tab is opened. -- work on the bindings.

executable.bind(needsDatabase(stateManager)); is the code search hint.

See other fixes for other cases: #12199

Copy link
Member

Choose a reason for hiding this comment

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

Maybe there is somewhere an instanceOf check missing where SaveDatabaseAction#save is called?

Comment on lines 148 to 158
if (tabContainer.getLibraryTabs().isEmpty()) {
return preferences.getFilePreferences().getWorkingDirectory();
} else {
Optional<Path> databasePath = tabContainer.getCurrentLibraryTab().getBibDatabaseContext().getDatabasePath();
return databasePath.map(Path::getParent).orElse(preferences.getFilePreferences().getWorkingDirectory());
}

LibraryTab currentTab = tabContainer.getCurrentLibraryTab();
if (currentTab == null) {
return preferences.getFilePreferences().getWorkingDirectory();
}

Optional<Path> databasePath = currentTab.getBibDatabaseContext().getDatabasePath();
return databasePath.map(Path::getParent).orElse(preferences.getFilePreferences().getWorkingDirectory());
Copy link
Member

Choose a reason for hiding this comment

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

getLibraryTabs is not empty, but getCurrentLibraryTab could be null?

fileHistory.setDisable(!hasRecentFiles);
});

this.welcomePage = new WelcomePage(
Copy link
Member

Choose a reason for hiding this comment

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

Why do you initialize this here? Just put all the WelcomePage stuff into the tab. You do not reuse the WelcomePage in JabRefFrame.

private final TaskExecutor taskExecutor;
private final FileHistoryMenu fileHistoryMenu;

public WelcomePage(JabRefFrame frame,
Copy link
Member

Choose a reason for hiding this comment

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

Always use the smalles possible scope to avoid unneccessary dependencies.

Suggested change
public WelcomePage(JabRefFrame frame,
public WelcomePage(LibraryTabContainer tabContainer,

import org.jabref.model.entry.BibEntryTypesManager;
import org.jabref.model.util.FileUpdateMonitor;

public class WelcomePage {
Copy link
Member

Choose a reason for hiding this comment

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

Integrate into WelcomeTab

Comment on lines 80 to 82
if (libraryTab == null) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there is somewhere an instanceOf check missing where SaveDatabaseAction#save is called?

@koppor
Copy link
Member

koppor commented Feb 10, 2025

For reference, the current VSCode welcome screen offers walkthroughs: - Maybe as follow-up

image

TaskExecutor taskExecutor,
FileHistoryMenu fileHistoryMenu) {

super("Welcome");
Copy link
Member

Choose a reason for hiding this comment

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

I think, there is a Localization missing?

Label welcomeLabel = new Label(Localization.lang("Welcome to JabRef"));
welcomeLabel.getStyleClass().add("welcome-label");

Label descriptionLabel = new Label(Localization.lang("Stay on top of your Literature"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Label descriptionLabel = new Label(Localization.lang("Stay on top of your Literature"));
Label descriptionLabel = new Label(Localization.lang("Stay on top of your literature"));

@@ -2854,3 +2854,11 @@ Include=Include
Exclude=Exclude
Include\ or\ exclude\ cross-referenced\ entries=Include or exclude cross-referenced entries
Would\ you\ like\ to\ include\ cross-reference\ entries\ in\ the\ current\ operation?=Would you like to include cross-reference entries in the current operation?
New\ Library=New Library
Copy link
Member

Choose a reason for hiding this comment

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

Very strange that a new localization is needed.

Please rework the strings to be in line with the other casing in JabRef

grafik

Copy link
Contributor

@github-actions github-actions bot left a 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 Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

@calixtus
Copy link
Member

Hi, I finally tested your changes in JabRef. I like it very much and I think its a great addition for new users with a big impact. I have two small suggestions from testing it:

  1. The welcomeTab should only open, if there is not already another file opened from last time (check for OpenDatabases object in uiCommands).
  2. Add a menu entry in help "open welcome tab".

Thanks!

@oops-shlok
Copy link
Contributor Author

@koppor Fixed the localisation issue. Anything else that needs to be changed?

@koppor
Copy link
Member

koppor commented Mar 19, 2025

@oops-shlok I will finish the PR - I have time only today for a longer JabRef session - and I think, this back and forth is not worth our time. Time is better spend in new issues.

What I did

  • Consistency in language: Help menu should have same strings than in the welcome tab. @InAnYan We changed both strings to "Community forum now"
  • The welcome tab was shown if a file was opened. I fixed this by placing the call at a better place.

@oops-shlok
Copy link
Contributor Author

@oops-shlok I will finish the PR - I have time only today for a longer JabRef session - and I think, this back and forth is not worth our time. Time is better spend in new issues.

What I did

  • Consistency in language: Help menu should have same strings than in the welcome tab. @InAnYan We changed both strings to "Community forum now"
  • The welcome tab was shown if a file was opened. I fixed this by placing the call at a better place.

Sure @koppor! I am also available now. You can lmk in case anything needs to be done.

@koppor koppor enabled auto-merge March 19, 2025 19:54
calixtus
calixtus previously approved these changes Mar 19, 2025
Copy link

trag-bot bot commented Mar 19, 2025

@trag-bot didn't find any issues in the code! ✅✨

@koppor
Copy link
Member

koppor commented Mar 19, 2025

Gui Tests are more fetcher tests - the remote seems to block - failing gui test is OK :)

@calixtus calixtus dismissed stale reviews from koppor, InAnYan, and subhramit March 19, 2025 20:36

obsolete

@koppor koppor added this pull request to the merge queue Mar 19, 2025
Merged via the queue into JabRef:main with commit 84513f7 Mar 19, 2025
31 of 33 checks passed
@calixtus
Copy link
Member

Finally this pr is merged. @oops-shlok thanks for your addition. A great new feature for JabRef.

@oops-shlok
Copy link
Contributor Author

Finally this pr is merged. @oops-shlok thanks for your addition. A great new feature for JabRef.

Thanks! It was great working on this feature. Will continue working on follow up PRs for this and also look into other issues. Thanks @calixtus and @koppor for the constant support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show a welcome screen if no database is open
5 participants