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

Fix option to "Search for unlinked local files" for new library #12738

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

raquelgraos
Copy link
Contributor

Disables the option to "Search for unlinked local files" in the Lookup menu before the newly created library is saved as the action should only be available when the .bib file path is known.

I changed the executable property binding to needsSavedLocalDatabase() to ensure that the action is only available under the defined conditions of the mentioned method.
I developed tests that assert that the action is only executable under those same conditions.

The video below shows the creation of a new library (without saving it) and how the "Search for unliked local files" is now disabled.

bug_fix.webm

Fixes #12558

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.

Disables the option to "Search for unlinked local files"
in the Lookup menu before the newly created library is
saved as the action shold only be available when the
.bib file path is known
Fixes JabRef#12558
@subhramit
Copy link
Member

Hi @raquelgraos, thank you for your contribution (and the tests).
On manually testing, the option seems to be disabled even after saving the library, unless JabRef is restarted.

I suppose this is not expected behavior? @koppor

@subhramit
Copy link
Member

Once this is resolved, I think the documentation should also be updated.

You can file a PR at https://github.com/JabRef/user-documentation.

@koppor
Copy link
Member

koppor commented Mar 13, 2025

Hi @raquelgraos, thank you for your contribution (and the tests). On manually testing, the option seems to be disabled even after saving the library, unless JabRef is restarted.

The video misses the part AFTER saving. - Meaning after end of the text

The action "Search for unlinked local files" should only run if the current library is saved (so that the path of the .bib file is known)."

I am sorry that I did not write down steps 6 7 8 there: SAving and then opening the menu agian.

@koppor
Copy link
Member

koppor commented Mar 13, 2025

Behavior in main:

image

Then save:

image


Too late here, don't have time to check the PR.

@subhramit
Copy link
Member

subhramit commented Mar 13, 2025

The video misses the part AFTER saving.

Yes, I was not referring to the video, but my attempt to test these changes locally:

the option seems to be disabled even after saving the library, unless JabRef is restarted.

@raquelgraos
Copy link
Contributor Author

Hi @subhramit @koppor ! Thank you for the feedback!

Upon further inspection, I believe the issue lies in the fact that the activeDatabaseProperty() is not notifying changes when saving the library. Immediately after saving (i.e. without restarting JabRef), if the active tab is switched and then switched back to the newly saved library, the option will be enabled.

@subhramit subhramit requested a review from calixtus March 14, 2025 00:01
@subhramit subhramit added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers component: ui labels Mar 14, 2025
@subhramit subhramit requested a review from Siedlerchr March 15, 2025 16:49
Siedlerchr
Siedlerchr previously approved these changes Mar 15, 2025
Siedlerchr
Siedlerchr previously approved these changes Mar 15, 2025
@subhramit
Copy link
Member

@Siedlerchr @calixtus any idea how to fix:

I believe the issue lies in the fact that the activeDatabaseProperty() is not notifying changes when saving the library. Immediately after saving (i.e. without restarting JabRef), if the active tab is switched and then switched back to the newly saved library, the option will be enabled.

This makes switching back and forth from libraries or restarting JabRef compulsory once the option is disabled (due to unsaved library), to enable it (even after saving).

@Siedlerchr Siedlerchr added this pull request to the merge queue Mar 15, 2025
@subhramit subhramit removed this pull request from the merge queue due to a manual request Mar 15, 2025
@subhramit
Copy link
Member

@Siedlerchr @calixtus any idea how to fix:

...

If not, I think a saner, more UX-friendly option is to keep the option enabled and show the user a dialog saying "The library needs to be saved." if they try to use it with an unsaved library.

@raquelgraos
Copy link
Contributor Author

Hi @subhramit and @Siedlerchr !
I am currently trying to figure out how saving the library could trigger a activeDatabaseProperty change which I believe would fix the issue.

But, if you believe this isn't the best route to fixing the issue, I could go back and implement that dialog you suggested.

@subhramit
Copy link
Member

subhramit commented Mar 15, 2025

But, if you believe this isn't the best route to fixing the issue, I could go back and implement that dialog you suggested.

If you have time you can start working on the dialog, otherwise I'd wait till @calixtus confirms that there is no other "easy"/"moderately easy" workaround.

@raquelgraos
Copy link
Contributor Author

Thank you for your reply!
I already managed to get the dialog working, but I will wait for more feedback.

@subhramit
Copy link
Member

subhramit commented Mar 16, 2025

Thank you for your reply! I already managed to get the dialog working, but I will wait for more feedback.

We had a discussion.
The ideal way to go about it would be to examine how this is solved in other places in JabRef. Take an example of any option that is disabled for unsaved libraries.

A hint could be event listeners. See this, for instance. That is how buttons are disabled or enabled in the open office panel when a style is chosen. There can be even better/simpler examples - this is the part I work on so came up in mind.

More hint: LibraryTab has a boolean changedProperty. Maybe this can be listened to.

@calixtus
Copy link
Member

Maybe a look into ActionHelper#isFilePresentForSelectedEntry can help in rewriting the method. The challenge is to listen to a property, that may not be present, if there is no library open.

@calixtus
Copy link
Member

calixtus commented Mar 16, 2025

A hint could be event listeners.

Sorry is was not clear, i did not meant event listeners, but the event bus.

@subhramit
Copy link
Member

subhramit commented Mar 16, 2025

Sorry is was not clear, i did not meant event listeners, but the event bus.

Oh no it's fine - I did mean event listener in that sentence. "Listener" and "event listener" are mostly used interchangeably in Java (I think?)

@raquelgraos
Copy link
Contributor Author

Thank you for your input!

More hint: LibraryTab has a boolean changedProperty. Maybe this can be listened to.

But the action should still be available even if some (unsaved) changes were made, am I correct?

The challenge is to listen to a property, that may not be present, if there is no library open.

I apologise, but I am not sure I completely understood this. When would the library not be open in this case?

@subhramit
Copy link
Member

But the action should still be available even if some (unsaved) changes were made, am I correct?

I think so. Point is to ensure that the library is tied to a saved path in the system to allow this.

I apologise, but I am not sure I completely understood this. When would the library not be open in this case?

That may have been a typo - maybe @calixtus meant a property which is not present when a library is not saved*

@subhramit subhramit removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 16, 2025
@subhramit subhramit added the status: changes required Pull requests that are not yet complete label Mar 16, 2025
@calixtus
Copy link
Member

It's about being notified as soon as the library is being saved. If the library is saved, changedProperty is set to false. As soon it is set to false, you can check if it has a stored path and the binding can be updated.
That does not mean, that changedProperty is meaning the same thing as the file being saved, but as far as I can see, it will be triggered as soon as soon as a library is saved.

@raquelgraos
Copy link
Contributor Author

raquelgraos commented Mar 17, 2025

Hi @calixtus! Unfortunately, I don't think the changedProperty in the libraryTab will help in this situation, considering it is set to false when a library is created and, therefore, when saved won't notify.

But, I am trying to figure out how to get around this situation.

@subhramit
Copy link
Member

But, I am trying to figure out how to get around this situation.

Yes, try a bit more. In case you're unable to find a way (in the interest of time and also considering we labeled this a "good first issue" but deeper hurdles came up), you can go forward with the workaround and open an issue stating "Improve UX accessing unlinked files option" describing how it'd be better if it could stay disabled for unsaved libraries, along with all the knowledge about properties required we've gathered so far.

@koppor
Copy link
Member

koppor commented Mar 19, 2025

I will merge to keep things going. In a follow-up when there was more code experience, maybe, the state manager thing can be fixed. Maybe, it's just an update of the state manager on an initial save.

Copy link

trag-bot bot commented Mar 19, 2025

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

@koppor koppor added this pull request to the merge queue Mar 19, 2025
Merged via the queue into JabRef:main with commit 523e90e Mar 19, 2025
33 checks passed
@raquelgraos
Copy link
Contributor Author

I will keep looking into it and if I find any new information I will share!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ui status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable "Search for unlinked local files" for new library
5 participants