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

Disable Write XMP Button in General tab of Entry-Editor when action is in progress #8728

Merged
merged 11 commits into from
May 30, 2022

Conversation

melmorsy
Copy link
Contributor

@melmorsy melmorsy commented Apr 26, 2022

Fixes #8691

Disable the button before starting the 'write' task then re-enables the button after the task is complete

  • 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.

Disable the button before starting the 'write' task then re-enables the button after the task is complete
@melmorsy melmorsy changed the title fixes #8691 fixes #8691 - Write XMP Button should be disabled when action is in progress #8691 Apr 26, 2022
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.

Could you maybe refactor the write xmp logic and implement it as a SimpleCommand? That way we can bind the disabled state in the view to the executable property of the SimpleCommand and benefit from the infrastructure the existing implementation of the command pattern provides.

@calixtus
Copy link
Member

calixtus commented May 6, 2022

Don't be overwhelmed. By refactoring the "write xmp logic" i just meant copy'n'paste the existing logic into a new class implementing SimpleCommand and bind the disabled-property of the button to the executable-property of the SimpleCommand.

@melmorsy
Copy link
Contributor Author

melmorsy commented May 22, 2022

Thanks @calixtus for your suggestion and apologies for the delayed reply. Using SimpleCommand and binding the properties looks certainly to be an interesting approach. I had a look at the code to understand SimpleCommand a bit more, however it wasn't clear to me how would it fit with BackgroundTask? Currently, writeMetadataToPdf creates a BackgroundTask and passes it to the taskExecutor, if we implement a SimpleCommand that does the same, then couldn't SimpleCommand's "execute" method return before the writing to PDF is actually complete, since writing to PDF is done as a BackgroundTask which could be on a different thread? i.e. the button might get enabled before writing to PDF is actually complete?

Or, shall the SimpleCommand be created first by moving the code from the BackgroundTask's runnable to SimpleCommand.execute, then bind SimpleCommand to the button's disabled state, then create a BackgroundTask something like this: BackgroundTask.wrap(()->simpleCommand.execute())?

Unfortunately I don't seem to find a documentation for SimpleCommand so my understanding is be based on going through the code usages for SimpleCommand, which may mean that I missed something, let me know please what do you think.

@calixtus
Copy link
Member

SimpleCommand is basically a wrapper for every ui action you can imagine.
Just put the backgroundTask into the execute method of a SimpleCommand and use the onRunning or onSuccess methods to update the executable property and bin the property to the disable-property of the button.

Mohamed El-Morsy and others added 8 commits May 23, 2022 22:18
Disable the button before starting the 'write' task then re-enables the button after the task is complete
Create WriteMetadataToPdfCommand, bind its 'executable' property to the button's 'disable' property
Disable the button before starting the 'write' task then re-enables the button after the task is complete
Create WriteMetadataToPdfCommand, bind its 'executable' property to the button's 'disable' property
Resolving merge conflict
Disable the button before starting the 'write' task then re-enables the button after the task is complete
Resolve checkstyle issues
Disable the button before starting the 'write' task then re-enables the button after the task is complete
Resolve checkstyle issues
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.

Looks good to me! Thanks for your contribution.

If you got time and want to go the extra mile you could also in a follow-up PR make the menu action use the new Command.

@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 24, 2022
@calixtus calixtus requested a review from Siedlerchr May 24, 2022 05:35
@ThiloteE
Copy link
Member

ThiloteE commented May 27, 2022

I tried this PR. I like that the button becomes grey for ~0,5 seconds, but when I double click, it opens the attached pdf, even though this button is not supposed to open the pdf. Also, when pressing the button multiple times, I get multiple notifications of having successfully written metadata to pdf, even though the button is still grey. Maybe I am not clicking fast enough haha :D

On the plus side: Have not encountered any error message (yet) and the attached PDF was not destroyed either (at least I think so :D) :-)

@ThiloteE ThiloteE changed the title fixes #8691 - Write XMP Button should be disabled when action is in progress #8691 Disable Write XMP Button in General tab of Entry-Editor when action is in progress May 27, 2022
@Siedlerchr Siedlerchr merged commit 1100526 into JabRef:main May 30, 2022
@Siedlerchr
Copy link
Member

Thanks for your contribution @melmorsy

Siedlerchr added a commit that referenced this pull request May 30, 2022
…rg.apache.lucene-lucene-queries-9.2.0

* upstream/main:
  Fix for removing several groups deletes only one of them (#8801)
  Disable Write XMP Button in General tab of Entry-Editor when action is in progress (#8728)
  Bump jsoup from 1.14.3 to 1.15.1 (#8864)
  Bump unirest-java from 3.13.8 to 3.13.10 (#8869)
  Bump unoloader from 7.3.2 to 7.3.3 (#8863)
  Bump pascalgn/automerge-action from 0.15.2 to 0.15.3 (#8860)
  Bump classgraph from 4.8.146 to 4.8.147 (#8861)
  Bump mockito-core from 4.5.1 to 4.6.0 (#8862)
Siedlerchr added a commit that referenced this pull request Jun 1, 2022
* upstream/main:
  Add an importer for Citavi backup files (#8848)
  Reviewdoc: Comment on PRs (#8878)
  Squashed 'buildres/csl/csl-styles/' changes from 649aac4..e740261
  Use JDK 15 text blocks to improve injected languages readability (#8874)
  Fix fetcher tests (#8877)
  Fix #8390 by allowing multiple group deletion for Remove groups > Kee… (#8875)
  Add restart warning on SSL configuration change (#8871)
  Update to lucene 9.2 (#8868)
  Fix for removing several groups deletes only one of them (#8801)
  Disable Write XMP Button in General tab of Entry-Editor when action is in progress (#8728)
  Bump jsoup from 1.14.3 to 1.15.1 (#8864)
  Bump unirest-java from 3.13.8 to 3.13.10 (#8869)
  Bump unoloader from 7.3.2 to 7.3.3 (#8863)
  Bump pascalgn/automerge-action from 0.15.2 to 0.15.3 (#8860)
  Bump classgraph from 4.8.146 to 4.8.147 (#8861)
  Bump mockito-core from 4.5.1 to 4.6.0 (#8862)
  Lucence dir checkers should only delete lucence dirs (#8854)
  Update README.md (#8858)
  Update adr.md
  Update adr.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external files status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write XMP Button should be disabled when action is in progress
4 participants