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

Changes in the entry editor mark the database as dirty #2997

Closed
wants to merge 2 commits into from

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Jul 11, 2017

Partial fix to #2787

Changes in the entry editor now mark the database as dirty. This are somewhat proactive: even only opening the entry editor will mark the database as changed. I'd say this isn't a problem. I'd rather have it do the marking than forget one.

Essentially, this adds another listener to the properties in the entry editor that propagates the change whenever the property changes. I haven't figured out how to do this for groups.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@lenhard lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 11, 2017
@stefan-kolb
Copy link
Member

I'd say this is a hack. Whereas it is better than missing a changed state for now, we should only mark the DB as dirty if we really changed something.

@tobiasdiez
Copy link
Member

I'm also not very happy with this solution from an architectural view: why should the gui handle the dirty/clean state and not the logic? I would propose to add listeners to the database/metadata in DatabaseContext which sets a corresponding flag. The UI can then listen to changes of this new property and display the star in the tab accordingly.

@Siedlerchr
Copy link
Member

The databaseChangedEvent is already realized with the EventBus system, so it just needs a couple of listeners if I understand it correctly. And one event bus can have multiple listeners for the same event

@lenhard
Copy link
Member Author

lenhard commented Jul 11, 2017

Ok, I see. So far it had been implemented only in the BasePanel, but I aggree that the cleanest solution is probably to track it in the database itself. This won't be as quick as the current solution, though :)

The consensus seems to be that we are not going in this direction, so I'll close this PR.

@lenhard lenhard closed this Jul 11, 2017
@stefan-kolb stefan-kolb deleted the entry-editor-dirty branch July 20, 2017 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants