Skip to content

Conversation

@danxuliu
Copy link
Member

Fixes #73
Fixes nextcloud/server#4869
Fixes nextcloud/server#6941
Fixes nextcloud/server#7499

When a new text file is created the editor is opened, which called getModelForFile and stored the returned model for later use.

However, FileInfoModels returned by getModelForFile are just temporary, and they should be used and forgotten instead of being kept. It is not guaranteed that different calls to getModelForFile for the same file returns the same FileInfoModel object, so changes in one model instance could be not known by other model instances (and their views). In fact, getModelForFile always returns a different model object, except when the file of _currentFileModel matches the file to get the model for.

_currentFileModel is set when the details view is updated to display certain file. It turns out that when any file is created the file list is scrolled to the new file and the details view for that file is opened. However, although the call to scrollTo happens before opening the editor the opening of the details view is defered, so it happens after the editor is opened. Thus, when a new text file was created, a FileInfoModel was created and kept by the editor and, then, a new FileInfoModel was created and stored in _currentFileModel.

The model stored by the editor is only used when the editor is closed. When that happens, the model is updated with the file properties changed by the editor (size, modification time). Changing a model causes the row for its file to be updated in the file list. And when a row is updated the old row is removed from the file list and a new one is added to replace it. But when there are several model instances for the same file and one of the instances is changed... the rest of instances do not know that the other instance was changed and that the row was updated! And here come the problems.

When the editor is closed its model instance is changed. But remember that there was another model instance around, the one created when the details view was shown, which was stored in _currentFileModel and that does not know that the file row was updated. See where this is going? Now, when getModelForFile is called again for that file the returned model is the one stored in _currentFileModel; if that model is then changed it will update the row... but it will try to remove the row that was already removed, so the current file row is not touched, and then it will add a new one, which will cause a duplicated entry to appear. Yay, you made it to the end of the explanation, congratulations :-P

This pull request modifies the text editor to use and forget the FileInfoModel instead of keeping it, which fixes the duplicated entries in the file list after creating a text file. The real fix would have to be done in the file list to ensure that getModelForFile always returns the same model instance for each file, but whatever the fix it should be backported to stable12 and stable13, and the one here is easier to backport ;-) (I have already made the backports, I will send them once this one is merged)

FileInfoModels returned by "getModelForFile" are just temporary, and
they should be used and forgotten instead of being kept by apps. It is
not guaranteed that different calls to "getModelForFile" for the same
file returns the same FileInfoModel object, so changes in one model
instance could be not known by other model instances (and their views),
and this could lead to different bugs (for example, a file list entry
being duplicated when a text file is edited or favourited after being
created).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, works and makes sense 👍

@MorrisJobke MorrisJobke merged commit 0a6f0bf into master Feb 14, 2018
@MorrisJobke MorrisJobke deleted the do-not-keep-fileinfomodels-returned-by-getmodelforfile branch February 14, 2018 16:23
@MorrisJobke
Copy link
Member

You rock @danxuliu ! 🚀

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

Projects

None yet

3 participants