Handlers for external changes or deletions #121

darahak · 2016-11-01T23:47:53Z

Proposal for #94

With this patch, Abricotine will display message boxes if the current file has been modified or deleted by a third party.
On changes, the user can choose to reload the file or leave it untouched.

In this case, or if the file was deleted, the document is forced to a "dirty" state to allow save, even on exit.

(Sorry about the noise with all the double quote changes)

…ted by another program.

…pen the proper dialog box if the app is closed without saving.

brrd

Thanks for your contribution!
I added some comments and questions for you in the diff.

brrd · 2016-11-02T10:04:10Z

app/renderer/abr-document.js

@@ -412,7 +478,7 @@ AbrDocument.prototype = {
    autopreview: function (types, lines) {
        var cm = this.cm;
        types = types || this.autopreviewTypes;
-        if (lines == null) {
+        if (lines === null) {


Please don't change == null. This is why .jshintrc includes the "eqnull": true rule.
See http://marijnhaverbeke.nl/blog/null-and-undefined.html for an explanation.

Thanks for the explanation! I'll rollback the change.

brrd · 2016-11-02T10:24:18Z

app/dialogs.js

            noLink: true
        });
+    },
+
+    warnFileDeleted: function (path, callback) {


I would rather display a warning here: "The file [path] doesn't exist anymore. Keep this file in editor?" => Yes/No
"No" closes the file, "Yes" sets the doc dirty and keep it open.

Makes sense.
I actually hesitated between these two behaviors.

Sublime Text simply changes the document state to "dirty", but Notepad++ does what you are describing.

brrd · 2016-11-02T10:36:34Z

app/renderer/abr-document.js

@@ -257,6 +262,10 @@ AbrDocument.prototype = {
        this.latestGeneration = this.getGeneration();
    },

+    setDirty: function () {
+        this.latestGeneration--;


I think this can lead to unexpected behaviors, especially when going back in the undo stack. I would rather set this.latestGeneration to -1 in order to be sure its value will never be the equal to CodeMirror's changeGeneration().

Good point.

The intention with -1 would also be clearer; decrementing latestGeneration looks like we are stepping back in the undo/redo stack when it's not the case.

brrd · 2016-11-02T10:40:56Z

app/renderer/abr-document.js

@@ -350,6 +366,65 @@ AbrDocument.prototype = {
        return this.save(path, callback);
    },

+    initWatcher: function () {
+        var that = this;
+        var updateTitle = function () {


Why don't you simply use abrDoc.updateWindowTitle() here instead of creating a duplicate of it?

From what I understand, updateWindowTitle() relies on CodeMirror's state to declare the document as clean or not.

Technically, since the document didn't change in CodeMirror, calling updateWindowTitle() here would not add the * symbol in the window title. I didn't try it though, so I might be wrong.

In the end, I just wanted a visual feedback for the user without forcing CodeMirror to change its state (maybe with side effects I don't know about).

Technically, since the document didn't change in CodeMirror, calling updateWindowTitle() here would not add the * symbol in the window title. I didn't try it though, so I might be wrong.

If you call setDirty() before updateWindowTitle() this should work.

I just wanted a visual feedback for the user without forcing CodeMirror to change its state

CodeMirror state won't change. Actually isClean() is just asking CodeMirror if latestGeneration matches the current state, so if you set latestGeneration to -1 this would do the job.

If you don't mind I'd prefer to keep this function (update title) unique, so if we need to change it in the future we won't forget anything.

Ok I see.
I didn't really like it anyway, because having two functions like that can be confusing.
I'll remove the duplicate function.

brrd · 2016-11-02T10:42:20Z

app/renderer/abr-document.js

+            unlink: function (path) {
+                dialogs.warnFileDeleted(path, function () {
+                    that.setDirty();
+                    updateTitle();


See my previous comment:

that.setDirty(); that.updateWindowTitle();

brrd · 2016-11-02T10:48:04Z

app/renderer/abr-document.js

+            }
+            that.startWatcher();
+        });
+        return true;


I think this return value is useless because the function is async (but it was always the case in my code, my mistake)

I only added it to stay consistent with saveAs().
Since it can return false if it gets an invalid path, I thought it should return true if the function doesn't exit early.

Should we get rid of these boolean returns altogether?

It makes sense, let's keep it like this.

brrd · 2016-11-02T10:49:20Z

app/renderer/abr-document.js

-        return files.writeFile(data, path, callback2);
+            data = this.getData();
+        files.writeFile(data, path, function (err) {
+            if (err) {


Maybe that.startWatcher(); should be triggered here?

Not sure which part you are referring to.

that.startWatcher() is triggered at the end of files.writeFile()'s callback, because the path should be updated before (done by that.setPath(path).

There is a return in the if (err) block. If this block is reached and the user cancels the "Save As" dialog then the watcher won't be started again.

I would do something like:

if (err) { that.startWatcher() return dialogs.fileAccessDenied(path, function () { that.saveAs(callback); }); }

Or maybe I'm missing something?

You're right; the watcher won't restart if the user cancels here.

But it looks weird if I just call that.startWatcher() twice in the same function.
It would be clearer if dialogs.fileAccessDenied() can return the user's choice instead of always false.
Then the additional call to that.startWatcher() would be justified in the code.

What do you think?

But it looks weird if I just call that.startWatcher() twice in the same function.

Actually it is not really called twice because of the return in the if (err) block. I see this block more like an separate error handler.

It would be clearer if dialogs.fileAccessDenied() can return the user's choice instead of always false.

Well, I don't really agree with this. In my opinion userChoice should remains internal to dialog functions (because we can exchange buttons position, which would change the returned index, by instance).

And in my opinion the watcher should be started before the "Save As" dialog is opened in order to clearly isolate the different operations (save, then save as).

Damn, I thought the additional call was before the if (err) block, sorry about that 😅
It totally makes sense then.

brrd · 2016-11-02T10:53:35Z

app/dialogs.js

@@ -31,7 +31,7 @@ var appDialogs = {
            options = {
                title: "About",
                message: "ABRICOTINE - MARKDOWN EDITOR (v. " + constants.appVersion + ")\n\nCopyright (c) 2015 Thomas Brouard\n\nThis program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later version.\n\nThis program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more details.\n\nYou should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>.",
-                buttons: ['OK'],
+                buttons: ["OK"],


Next time could you correct coding style issues in a separate commit please? This would make the diff easier to read.

Yes, sorry about that 😞
I'll be more careful next time.

darahak · 2016-11-02T12:38:01Z

Thanks for taking the time to review this 👍
I'll make some changes to take your comments into account.

…e file in the editor, instead of just displaying a warning.

…tement in case of access denied on save.

nloveladyallen · 2016-11-02T22:33:02Z

The behavior here is a bit odd.

I'm getting two popups when the file is modified by another process, one after another, both saying the same thing. One is a dialog box (i.e. separate window) and one is a sheet (i.e. in the main window). If I say "Yes" to either (or both), the file is reloaded.

When deleting a file, I'm getting one dialog immediately, saying the file no longer exists. This seems to be working as intended. However, on every subsequent save of the file, after the file is written to disk, Abricotine shows a sheet saying that another process has modified the file (identical to the second screenshot above) and asking to reload.

Also, I occasionally get the following error when a file is modified by another process or saved by Abricotine:

It seems to happen randomly, and I can't reproduce it reliably. Once it happens once, it happens every time I try to save the file, and I have to close the window and re-open to get rid of it.

darahak · 2016-11-02T22:45:34Z

@nloveladyallen The first issue occurs when multiple changes are made by another process, I'll prepare a fix.

The second one should not happen anymore with the latest commits.
I got rid of the function at the origin of the error.

…uring file reload dialogs to avoid multiple spawns.

brrd · 2016-11-26T16:07:29Z

@darahak Are you still working on this PR or is it ready to merge?

darahak · 2016-11-26T16:43:56Z

I'm not working on it anymore.
I think you can merge it unless we find other issues (I addressed all the comments).

brrd · 2016-11-26T18:35:35Z

👍

darahak added 4 commits November 1, 2016 23:50

File monitoring: warns the user if the file has been modified or dele…

ee22013

…ted by another program.

Stop the watch listeners when the document is closed.

f8d9e6f

Merged with 57cd825.

1649245

Declare the document as "dirty" after external change or delete, to o…

f81df22

…pen the proper dialog box if the app is closed without saving.

brrd reviewed Nov 2, 2016

View reviewed changes

darahak added 2 commits November 2, 2016 23:19

Changed dialogs.warnFileDeleted() to ask if the user wants to keep th…

0ae20b4

…e file in the editor, instead of just displaying a warning.

Removed duplicate updateTitle() function and added startWatcher() sta…

0ad947c

…tement in case of access denied on save.

Stop watching files that don't exist anymore, and pause the watcher d…

af35dac

…uring file reload dialogs to avoid multiple spawns.

brrd merged commit fd2d0e6 into brrd:develop Nov 26, 2016

brrd mentioned this pull request Nov 26, 2016

Don't steal focus when file has changed #132

Closed

darahak deleted the auto-reload branch November 27, 2016 19:07

brrd mentioned this pull request Jun 7, 2017

Saving is broken on OS X #174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handlers for external changes or deletions #121

Handlers for external changes or deletions #121

darahak commented Nov 1, 2016

brrd left a comment

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016 •

edited

Loading

brrd Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

brrd Nov 2, 2016

darahak Nov 2, 2016

darahak commented Nov 2, 2016

nloveladyallen commented Nov 2, 2016

darahak commented Nov 2, 2016

brrd commented Nov 26, 2016

darahak commented Nov 26, 2016

brrd commented Nov 26, 2016

Handlers for external changes or deletions #121

Handlers for external changes or deletions #121

Conversation

darahak commented Nov 1, 2016

brrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darahak Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darahak commented Nov 2, 2016

nloveladyallen commented Nov 2, 2016

darahak commented Nov 2, 2016

brrd commented Nov 26, 2016

darahak commented Nov 26, 2016

brrd commented Nov 26, 2016

darahak Nov 2, 2016 •

edited

Loading