-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add more walkthroughs, donation prompt, and responsive layout for welcome tab #13679
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
Conversation
jabgui/src/main/java/org/jabref/gui/walkthrough/declarative/NodeResolver.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/welcome/components/DonationProvider.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/welcome/components/DonationProvider.java
Show resolved
Hide resolved
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, need to look more in detail
jabgui/src/main/java/org/jabref/gui/walkthrough/declarative/NodeResolver.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/walkthrough/declarative/NodeResolver.java
Show resolved
Hide resolved
@Yubo-Cao Once you address comments, before marking them as resolved, a good way to check if you have forgotten to push changes is to see if the review window is marked "Outdated". |
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.
Okay, logic-wise PR looks good
Just remove the extra newlines at the end of some files that are present
...n/java/org/jabref/gui/walkthrough/declarative/sideeffect/EnsureSearchSettingsSideEffect.java
Show resolved
Hide resolved
public DonationPreferences getDonationPreferences() { | ||
DonationPreferences dp = new DonationPreferences(getBoolean(DONATION_NEVER_SHOW), getInt(DONATION_LAST_SHOWN_EPOCH_DAY)); | ||
EasyBind.listen(dp.neverShowAgainProperty(), (_, _, nv) -> putBoolean(DONATION_NEVER_SHOW, nv)); | ||
EasyBind.listen(dp.lastShownEpochDayProperty(), (_, _, nv) -> putInt(DONATION_LAST_SHOWN_EPOCH_DAY, nv.intValue())); | ||
return dp; | ||
} |
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.
DonationPreferences does not use the current cache pattern, which is okay. But it needs to be documented why. Please add a comment.
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 prefer to have it consistent with other preferences in case we want to reuse it
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.
Fixed by adding the cache pattern
jabgui/src/main/java/org/jabref/gui/search/GlobalSearchBar.java
Outdated
Show resolved
Hide resolved
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.
Took another look, a few small things
Please also take a look at the failing jabgui
tests
private final SearchDisplayMode previousDisplayMode; | ||
|
||
public EnsureSearchSettingsSideEffect() { | ||
var preferences = Injector.instantiateModelOrService(GuiPreferences.class).getSearchPreferences(); |
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.
- avoid
var
- name it
searchPreferences
- make it a class-level variable. Since it is being initialized here in the constructor, use it everywhere below instead of getting each time in the methods where used
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.
Better be explicitly injected in the constructor arguments.
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.
Fixed
DonationPreferences dp = new DonationPreferences(getBoolean(DONATION_NEVER_SHOW), getInt(DONATION_LAST_SHOWN_EPOCH_DAY)); | ||
EasyBind.listen(dp.neverShowAgainProperty(), (_, _, nv) -> putBoolean(DONATION_NEVER_SHOW, nv)); | ||
EasyBind.listen(dp.lastShownEpochDayProperty(), (_, _, nv) -> putInt(DONATION_LAST_SHOWN_EPOCH_DAY, nv.intValue())); | ||
return dp; |
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.
Expand abbreviations dp and nv.
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.
Fixed
Can you take a look at the failing test, Yubo |
Got it! The JabRef GUI test used to be always giving out errors (at the beginning), so I almost grow immune to the red checkmarks on it. It turns out that my change required a few more mock parameter to be added to the search preferences, so I modified the test slightly and got that to work. |
@trag-bot didn't find any issues in the code! ✅✨ |
As soon as the issue @Siedlerchr raised is cleared, this is imho ready to be merged. |
JUnit tests of 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. |
Closes #12664
This PR introduces walkthroughs for add group and search in the library, responsive layout for welcome tab (to make sure the community links footer is always visible), and a donation popup to be shown 7 days after first launch, every 365 days.
Steps to test
Resize the window to show responsive layout
Click on the walkthroughs buttons to try them out
Try out donation popup is rather challenging. Modify the code to add a button in the
WelcomeTab
is one possible way to perform this, as shown below:Or alternatively, if you are using Windows,
Win+R
and type inregedit
to open the registry editor. Change the value ofHKEY_CURRENT_USER\Software\JavaSoft\Prefs\org\jabref\donation/Last/Shown/Epoch/Day
to0
will trigger donation prompt to be shown upon launch.You can find those feature demos here:
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)