Skip to content

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

Merged
Siedlerchr merged 11 commits into
JabRef:mainfrom
melmorsy:fix-for-issue-8691
May 30, 2022
Merged

Disable Write XMP Button in General tab of Entry-Editor when action is in progress#8728
Siedlerchr merged 11 commits into
JabRef:mainfrom
melmorsy:fix-for-issue-8691

Conversation

@melmorsy

@melmorsy melmorsy commented Apr 26, 2022

Copy link
Copy Markdown
Contributor

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

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

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

calixtus commented May 6, 2022

Copy link
Copy Markdown
Member

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

melmorsy commented May 22, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

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

ThiloteE commented May 27, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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

component: external-files component: ui status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

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