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

timestamp is not updated when entry changes #2810

Closed
ilippert opened this Issue May 1, 2017 · 13 comments

Comments

Projects
None yet
9 participants
@ilippert

ilippert commented May 1, 2017

JabRef 4.0.0-dev--snapshot--2017-05-01--master--fcac52707
Linux 4.10.12-200.fc25.x86_64 amd64
Java 1.8.0_131

as noted in the opening post of #2808, in my version the timestamp is not updated. even though I configured it to be updated (and it worked in recent versions)
screenshot mon may 01 2017 20 39 04 gmt 0200 cest 1145x75

The user experience is related to #2812: to some degree the changed timestamp is simply not immediately shown.

However, for some entries (and I do not recognise a pattern yet) the timestamp is not updated at all (i.e. even no change of timestamp after change and save and restart of jabref.

@tobiasdiez tobiasdiez added the bug label May 2, 2017

@koppor koppor added the beginner label May 4, 2017

@tobiasdiez tobiasdiez changed the title from timestamp is not updated to timestamp is not updated when entry changes May 4, 2017

@ayanzunaid

This comment has been minimized.

Show comment
Hide comment
@ayanzunaid

ayanzunaid May 6, 2017

I would like to work on this issue.

ayanzunaid commented May 6, 2017

I would like to work on this issue.

@koppor

This comment has been minimized.

Show comment
Hide comment
@koppor

koppor May 6, 2017

Member

@tobiasdiez I think that it makes sense that @ayanzunaid works on that. I suppose, the code has to be adapted to use our eventing system?

Member

koppor commented May 6, 2017

@tobiasdiez I think that it makes sense that @ayanzunaid works on that. I suppose, the code has to be adapted to use our eventing system?

@Siedlerchr

This comment has been minimized.

Show comment
Hide comment
@Siedlerchr

Siedlerchr May 6, 2017

Contributor

I took a quick look yesterday and it seems that only the file timestamp is somehow updated (according to the config options), and not the entry timestamp in the main table

Contributor

Siedlerchr commented May 6, 2017

I took a quick look yesterday and it seems that only the file timestamp is somehow updated (according to the config options), and not the entry timestamp in the main table

@tobiasdiez

This comment has been minimized.

Show comment
Hide comment
@tobiasdiez

tobiasdiez May 6, 2017

Member

@ayanzunaid you are of course welcome to work on it. The root of the issue is probably that the doUpdateTimestamp method is no longer called with the recent changes in the entry editor.
Moreover, I think it would make sense that this method is extracted from the entry editor and implemented as a change listener for the whole database (so that also changes via e.g. the cleanup operation) trigger an update of the timestamp.

Feel free to ask for further help if you get stuck at some point.

Member

tobiasdiez commented May 6, 2017

@ayanzunaid you are of course welcome to work on it. The root of the issue is probably that the doUpdateTimestamp method is no longer called with the recent changes in the entry editor.
Moreover, I think it would make sense that this method is extracted from the entry editor and implemented as a change listener for the whole database (so that also changes via e.g. the cleanup operation) trigger an update of the timestamp.

Feel free to ask for further help if you get stuck at some point.

@ferg35

This comment has been minimized.

Show comment
Hide comment
@ferg35

ferg35 Jun 27, 2017

hello. i want to help with this issue, but cant find where recent changes in the entry editor saves.
is there any listener for this? where would you advise me to watch? (sorry for my english)

ferg35 commented Jun 27, 2017

hello. i want to help with this issue, but cant find where recent changes in the entry editor saves.
is there any listener for this? where would you advise me to watch? (sorry for my english)

@tobiasdiez

This comment has been minimized.

Show comment
Hide comment
@tobiasdiez

tobiasdiez Jun 27, 2017

Member

@ferg35: Nice to hear!

The EntryChangedEvent is issued after an entry changed (see here for a list of uses of this class). You can subscribe to these events similar to the SearchAutoCompleteListener. As I said above, I wouldn't tie the update to the entry editor since entries can be changed also on other means (e.g. cleanup).

Member

tobiasdiez commented Jun 27, 2017

@ferg35: Nice to hear!

The EntryChangedEvent is issued after an entry changed (see here for a list of uses of this class). You can subscribe to these events similar to the SearchAutoCompleteListener. As I said above, I wouldn't tie the update to the entry editor since entries can be changed also on other means (e.g. cleanup).

@Siedlerchr

This comment has been minimized.

Show comment
Hide comment
@Siedlerchr

Siedlerchr Jun 27, 2017

Contributor
Contributor

Siedlerchr commented Jun 27, 2017

@lhoff94

This comment has been minimized.

Show comment
Hide comment
@lhoff94

lhoff94 Nov 7, 2017

hi
it seems like that this Issue is still not fixed. I assume that there is currently nobody working on it right now. If thats the case i'd like to work on that Issue together with @tobous.

lhoff94 commented Nov 7, 2017

hi
it seems like that this Issue is still not fixed. I assume that there is currently nobody working on it right now. If thats the case i'd like to work on that Issue together with @tobous.

@koppor

This comment has been minimized.

Show comment
Hide comment
@koppor

koppor Nov 7, 2017

Member

@bimini94 Yes, please. Go ahead. - I think, the EntryChangedEvent is not fired that often now. I think @lenhard did something there in the context of autosave.

Member

koppor commented Nov 7, 2017

@bimini94 Yes, please. Go ahead. - I think, the EntryChangedEvent is not fired that often now. I think @lenhard did something there in the context of autosave.

@tobous

This comment has been minimized.

Show comment
Hide comment
@tobous

tobous Nov 13, 2017

Contributor

We had a look at the problem. The needed functionality seems to already be present in EntryEditor.StoreFieldAction#updateTimeStamp(UndoableEdit). The problem is that the class EntryEditor.StoreFieldAction is never used. Furthermore, the EntryEditor is no longer registering the annotated "@subscribe" methods after commit 42ea6bc (which is part of a fix for issue #3366).

We would like to follow the given suggestions and create a listener on EntryChangedEvent which is listening on the whole database.
We still have some questions regarding the implementation of the listener:

  • Our current plan is to implement the listener as an inner class of BasePanel.java and then call "this.getDatabase().registerListener(...)" with our listener. This would follow the given example of the SearchAutoCompleteListener.
    Would this be acceptable or should our listener be contained in a different class or be registered on a different EventBus?

  • Our fix would basically follow the solution implemented in EntryEditor.StoreFieldAction#updateTimeStamp(UndoableEdit). In this method, everything is handled as an UndoableEdit. Is the usage of an UndoableEdit important for the setting of the timestamp?
    Our current guess would be that it is only an UndoableEdit in the existing implementation so that it can be reversed if the change that caused the event is reversed. This would however not apply at a database level as there are different causes that are not all undoable. Or should our implementation include a check to make our change of the timestamp undo-able if the change that caused the event is undoable?

Furthermore, we would like to address the frequency of fired events: During our tests, the EntryChangedEvent was still fired with every added (or removed) character. We haven't seen any events triggered in any other context (for example by autosaves). But we did not do very extensive testing, so this doesn't have to mean much.

Contributor

tobous commented Nov 13, 2017

We had a look at the problem. The needed functionality seems to already be present in EntryEditor.StoreFieldAction#updateTimeStamp(UndoableEdit). The problem is that the class EntryEditor.StoreFieldAction is never used. Furthermore, the EntryEditor is no longer registering the annotated "@subscribe" methods after commit 42ea6bc (which is part of a fix for issue #3366).

We would like to follow the given suggestions and create a listener on EntryChangedEvent which is listening on the whole database.
We still have some questions regarding the implementation of the listener:

  • Our current plan is to implement the listener as an inner class of BasePanel.java and then call "this.getDatabase().registerListener(...)" with our listener. This would follow the given example of the SearchAutoCompleteListener.
    Would this be acceptable or should our listener be contained in a different class or be registered on a different EventBus?

  • Our fix would basically follow the solution implemented in EntryEditor.StoreFieldAction#updateTimeStamp(UndoableEdit). In this method, everything is handled as an UndoableEdit. Is the usage of an UndoableEdit important for the setting of the timestamp?
    Our current guess would be that it is only an UndoableEdit in the existing implementation so that it can be reversed if the change that caused the event is reversed. This would however not apply at a database level as there are different causes that are not all undoable. Or should our implementation include a check to make our change of the timestamp undo-able if the change that caused the event is undoable?

Furthermore, we would like to address the frequency of fired events: During our tests, the EntryChangedEvent was still fired with every added (or removed) character. We haven't seen any events triggered in any other context (for example by autosaves). But we did not do very extensive testing, so this doesn't have to mean much.

tobous added a commit to lhoff94/jabref that referenced this issue Nov 14, 2017

Fix timestamps not being updated for changed entries
This is a fix for the issue JabRef#2810.

The functionality has been moved from the EntryEditor to the BasePanel
class so that it can listen for EntryChangedEvents on the whole
database instead of specific EntryEditor objects. This allows us to
also update the timestamps of entries that were changed by other
sources (like cleanup operations).

The inner class UpdateTimestampListener is defined as "public static"
so that it can be used in the test cases for the implemented
functionality.

@tobous tobous referenced this issue Nov 14, 2017

Merged

Fix for issue 2810 #3433

3 of 3 tasks complete
@tobous

This comment has been minimized.

Show comment
Hide comment
@tobous

tobous Nov 14, 2017

Contributor

Created a WIP pull request under the above mentioned assumptions.

We have some problems with our new tests being dependent on JabRefPreferences, which is not allowed according to the test TestArchitectureTests#testsAreIndependent(). A more detailed description is given as part of the pull request.

Contributor

tobous commented Nov 14, 2017

Created a WIP pull request under the above mentioned assumptions.

We have some problems with our new tests being dependent on JabRefPreferences, which is not allowed according to the test TestArchitectureTests#testsAreIndependent(). A more detailed description is given as part of the pull request.

tobiasdiez added a commit that referenced this issue Nov 27, 2017

Fix for issue 2810 (#3433)
* Fix timestamps not being updated for changed entries

This is a fix for the issue #2810.

The functionality has been moved from the EntryEditor to the BasePanel
class so that it can listen for EntryChangedEvents on the whole
database instead of specific EntryEditor objects. This allows us to
also update the timestamps of entries that were changed by other
sources (like cleanup operations).

The inner class UpdateTimestampListener is defined as "public static"
so that it can be used in the test cases for the implemented
functionality.

* Add tests for UpdateTimestampListener

* Move UpdateTimestampListenerTest to gui package

* Move UpdateTimestampListener into separate class

* Fix assertions for Optional values in UpdateTimestampListenerTest

* Add blank lines in UpdateTimestampListenerTest

The blank lines should help differentiate between the constants that
the test cases share and the constant that is different in both test
cases.

* Move shared calls into setup in UpdateTimestampListenerTest

This only applies to shared calls that are not dependent on the used
constants. As these constants define the values used in each test,
they are kept in each test case to improve readability. This also
applies to constants that have the same value in all tests.

* Use camel case for constants in UpdateTimestampListenerTest

* Add UpdateTimestampListenerTest to architecture test exceptions

This allows the added test cases to be dependent on JabRefPreferences
without causing TestArchitectureTests#testsAreIndependent() to fail.

The exception is warranted as the tested functionality is dependent on
JabRefPreferences and the test cases mock all calls to it.
@tobiasdiez

This comment has been minimized.

Show comment
Hide comment
@tobiasdiez

tobiasdiez Nov 27, 2017

Member

This should be fixed in the latest development version thanks to a fix by @tobous. Could you please check the build from http://builds.jabref.org/master/. Thanks!

Member

tobiasdiez commented Nov 27, 2017

This should be fixed in the latest development version thanks to a fix by @tobous. Could you please check the build from http://builds.jabref.org/master/. Thanks!

@tobiasdiez tobiasdiez closed this Nov 27, 2017

@ilippert

This comment has been minimized.

Show comment
Hide comment
@ilippert

ilippert Nov 28, 2017

Yeah, it works! Thanks a lot!!!

ilippert commented Nov 28, 2017

Yeah, it works! Thanks a lot!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment