Recent docs #92

cicithesquirrel · 2016-07-10T17:10:53Z

Fix for #69.

What's to be done:

Store the recent docs list in the config (max length of this list is in the config, too),
Recent docs list gets updated when the user opens or saves a file,
Add a "File > Recent" menu that shows the recent docs' paths (clicking an item will open the file),
Add a "File > Recent > Clear" menu to clear the recent docs list.

cicithesquirrel · 2016-07-10T17:11:07Z

Current progress is:

The first opened doc is not saved (but why?!)
"File > Recent" menu is present but not filled/implemented

brrd · 2016-07-10T18:09:39Z

Hi,

Thanks again for your contribution.

I think you should have a look at this for a better desktop integration on windows and osx.

In my opinion, recent docs history should not be stored in config.json for privacy reasons. Maybe not even in the config dir. What do you think about using localStorage API instead? (so everything would be done in renderer process)

The first opened doc is not saved (but why?!)

Maybe because of interferences between main and renderer processes (each of them are reading/writing in the config). Have you tried to call storeRecentPath(this.path, this.id) earlier? Like before abrWin.open() is called in abrWin constructor?

cicithesquirrel · 2016-07-11T20:38:33Z

I have troubles having a "recent files" menu that is dynamically filled and updated. This may be related to an Electron bug/limitation (electron/electron#528)...

I found that I can:

myMenu.clear() to destroy all of its items
myMenu.insert(0, new MenuItem({...})) to insert an item

But, the clear method is not documented in Electron API and I am worried I could create memory leaks using it 😟

I also tried to pre-create the menu items and then change their labels when the recent files list has changed. But it didn't work out: labels do not change.

…after closing empty window

cicithesquirrel · 2016-09-09T05:58:20Z

app/abr-menu.js

+
+    openRecentDoc: function (recentFile) {
+        this.abrWin.abrApp.open(recentFile);
+        // FIXCC recent-docs: Recent doc list is not updated after re-opening a recent file


Remaining FIX/TODO: update recent doc list when reopening a recent file.

brrd · 2016-11-03T10:37:02Z

I this PR ready for a review? I must confess that I lost track, all those updates confused me a bit 😕

cicithesquirrel · 2016-11-04T07:07:04Z

Yes, this PR is quite large...

In my opinion, there are two small things that could be fixed, even though they may be not essential:

A small bug fix (see comment in code)
An i18n label but merging i18n branch in develop is needed for this one.

But there is one thing that annoys me more: it is that I introduced a menu whose items change over time. This seems to go against Electron possibilities, and I was forced to use undocumented APIs (see my comment above).

For fear of instability, one alternative would be to limit this feature to simple OS integration. As the doc says, it works on Windows (confirmed on Windows 10) and Mac (I cannot test this). On Ubuntu 16, there is no menu for selecting a recent doc from the taskbar, but the recent doc is listed in the global "recent files" menu, so it quite works...

So there is one decision to make: keep this PR "as is", or trash most of its lines of code to keep what is sure to work in the long term. That's a hard decision to make, and I may not be really objective ;-) So, tell me what you think!

darahak · 2016-11-04T20:19:02Z

VS Code has an "Open Recent" sub-menu which is similar to what you did.
It works fine, I don't see the problem.

I think this is a must-have, even though OS integration could be a nice addition.
FWIW, I ended up disabling it because I never used it (not intuitive for me).

brrd · 2016-11-05T11:01:23Z

First of all: thank you for this contribution.

Well, in my opinion we should only use OS integration and avoid the "open recent" menu for several reasons:

As you pointed it out, dynamically updating menu items in electron is not easy (see this and this). It's even more difficult when we must do it across windows. I don't say it's impossible, but it is tricky.
We don't know if using menuItem.clear() is safe in term of performance and if it'll remain it electron API. I don't really like this...
We still have to try your code on OSX and I'm not sure at all that it'll work because OSX menu bar behaves different in many ways.

Actually OS recent documents is easier to manage for us and it respects user's privacy settings. I personally don't like recent docs recording and I'm glad when I can switch it off with a single setting, which is the case in most OS.

So my idea would be to simply use OS integration right now and to wait for a better support of recent docs in Electron (or for some community module which would do the job in a reliable way) to create a dedicated menu.

But this is just my opinion, and as I told I don't use this feature. If you really think a menu would be useful then let's add it. I have to say that in such case we will have to work a bit more on it because I don't really agree with some parts of your code.

cicithesquirrel · 2016-11-07T18:38:08Z

To me, the best argument for not doing this is that Electron does not officially support this yet.

In my point of view, a "recent docs" menu is an interesting feature: OS integration is usually not enough for me (but I wouldn't say it is a "must-have"). Your privacy concerns are understandable, and an option for enabling/disabling the menu could be useful. And the "clear" menu item can do the rest.

The option is also a way of avoiding any bugs on OSX, waiting for someone to test and contribute any patch that is needed.

As pointed by @darahak, we could look deeper into VSCode's way of doing this (in fact, I should have done this at first...).

That being said, if you still don't feel like merging this PR, it's OK with me. It won't hurt me or discourage my contributing to this project :-)

brrd · 2016-11-08T08:55:47Z

OK, it's useful so let's do it!

As pointed by @darahak, we could look deeper into VSCode's way of doing this (in fact, I should have done this at first...).

Currently I see 2 ways to do this in electron:

Use clear() as you did.
Reprocess the whole menu bar each time the menu is updated (for each window). But this is a totally different approach and would better keep what you wrote for now in my opinion.

I need to review this PR first. I'm busy in this time so it'll wait a bit 😕

Then we'll run tests on Windows/OSX/Linux. I got a VM for each OS now so I could maybe help.

brrd

I took an some time to read your code and I dropped my comments about it.

My main observation is that using the localStorage API to store filepaths actually leads to several issues. For instance this won't work if there is no open window because localStorage is a browser-specific API. Therefor clear() won't work in some cases on OSX.

I'm really sorry because I'm the one who suggested to use localStorage, but obviously we should have thought more about this at the beginning 😕

I still think that config.json is not the good place to store recent paths, but maybe we could use another file, let's say recent.json, in the same directory. This way the code would also be easier to read because most operations (such as writting/reading JSON and updating/clearing menu) would be done in the main process while abrDocument methods would only trigger calls to abrApplication.

What do you think?

brrd · 2016-11-26T14:32:24Z

app/abr-application.js

+        if (recentPaths) {
+            if (recentPaths.length > 0) {
+                // May not work on all OS'es (electron doc says Windows and MacOS are OK, tests on Ubuntu 16 show it is OK too)
+                // See: https://github.com/electron/electron/blob/master/docs/tutorial/desktop-environment-integration.md#recent-documents-windows--macos


No need to document this here.

brrd · 2016-11-26T14:32:49Z

app/abr-application.js

+                //      1. Start Abricotine ==> Opens a new empty window
+                //      2. Open one of the recent files using the "File > Recent" menu ==> Selected doc is opened in a new window (unlike the "File > Open menu")
+                //      3. Close the empty window (the one opened when Abricotine started)
+                //      4. Return the window opened at step 2, open a file using the "File > Open" menu


I think this comment is useless, code is clear enough.

brrd · 2016-11-26T14:33:00Z

app/abr-application.js

+    },
+
+    clearRecentDocs: function(abrWin) {
+        // See: https://github.com/electron/electron/blob/master/docs/tutorial/desktop-environment-integration.md#recent-documents-windows--macos


Same comment here. We won't add links to electron documentation for each method we use.

brrd · 2016-11-26T14:35:04Z

app/renderer/abr-document.js

@@ -19,9 +19,11 @@ var remote = require("electron").remote,
    shell = require("electron").shell,
    spellchecker = require('spellchecker');

-function AbrDocument () {
+function AbrDocument (theLocalStorage) {


This theLocalStorage argument is totally confusing. Why don't you simply use localStorage ?

brrd · 2016-11-26T14:38:06Z

default/config.json

@@ -8,7 +8,8 @@
    },
    "autopreview-security": true,
    "editor": {
-        "font-size": "16px"
+        "font-size": "16px",
+        "max-recent": 5


max-recent should not be stored under editor.
editor contains the text editor control basic configuration.

brrd · 2016-11-26T14:46:51Z

app/renderer/abr-document.js

+    updateRecentPath: function (path) {
+        var thisDoc = this;
+
+        this.getConfig(undefined, function(theConfig) {


The correct way to do is:

this.getConfig("editor:max-recent", function(max) {

brrd · 2016-11-26T14:47:49Z

app/renderer/abr-document.js

+    clearRecentDocs: function() {
+        var thisDoc = this;
+
+        this.getConfig(undefined, function(theConfig) {


No need to get the config here.

cicithesquirrel · 2016-12-01T06:29:40Z

That's OK with me. Your "small" change requests have been fixed, I still need to work on the "storage". I'll keep you posted.

brrd · 2016-12-01T08:28:44Z

app/renderer/main.js

@@ -7,6 +7,6 @@
 var AbrDocument = require.main.require("./abr-document.js");

 $( function () {
-    var abrDoc = new AbrDocument();
+    var abrDoc = new AbrDocument(localStorage);


Good! Don't forget this one.

Sorry, I missed that one... Fixed.

nettnikl · 2020-11-10T16:07:58Z

Is this merge request still in work, or is it blocked some other issue?

brrd · 2020-11-11T09:35:42Z

@nettnikl This PR had no activity for almost 4 years and the related branch is 335 commits behind the current develop branch, so we can consider it obsolete unfortunately.

I'm closing this thread.

cicithesquirrel added 3 commits July 10, 2016 17:06

Store recent files in config

1ab38e1

Added submenu for recent docs

a9c98ee

Added TODO

66339d2

cicithesquirrel added 6 commits July 10, 2016 20:40

Added TODO

42742ca

Added MacOS and Winsows integration

76ab14a

Fix: bad "require"

5468543

Recent docs are now stored in the browser localStorage

016b982

(beginning) update the recent-docs menu

7b4bad8

Update windows' menus

1f8543b

cicithesquirrel added 13 commits July 12, 2016 22:28

Clear and recrate the "recent docs" menu

ed46e67

Removed console log

ad3a65c

Update "recent docs" menu on when the window is opened

49fc653

Removed unused code

026267b

Re-open a recent file

2be8edc

Remove max-recent config value

1d04673

Clear recent docs (beginning)

33c8ff8

Remove console log

190b823

Clearing the recent docs menu

1e119ef

Update recent paths menu when selecting one of the recent files

9c4f91f

Fix: Error when closing document on empty window & when opening file …

5b01ed6

…after closing empty window

Fix: Label of menu "File > Recent > Clear"

b076fb2

Enhance comment

73f4b69

cicithesquirrel reviewed Sep 9, 2016
View reviewed changes

brrd mentioned this pull request Nov 10, 2016

Change default paths for Save and Export #129

Closed

brrd suggested changes Nov 26, 2016

View reviewed changes

cicithesquirrel added 7 commits November 28, 2016 08:18

Moved TODO

e5f5c6b

Remove comments

38a6999

Better way of getting the config

21f4780

Use global localStorage

de8881c

Fix: debug is always enabled

99d9bdb

Change "max-recent" location in config

d5af0df

No need to get config

8cfab90

brrd reviewed Dec 1, 2016

View reviewed changes

Remove unused parameter

8311eb7

brrd closed this Nov 11, 2020

nettnikl mentioned this pull request Nov 11, 2020

Recent docs #293

Closed

granbosque added a commit to granbosque/abricotine that referenced this pull request Nov 24, 2022

Recent documents (based on brrd#92)

563c39f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recent docs #92

Recent docs #92

cicithesquirrel commented Jul 10, 2016

cicithesquirrel commented Jul 10, 2016

brrd commented Jul 10, 2016

cicithesquirrel commented Jul 11, 2016 •

edited

Loading

cicithesquirrel Sep 9, 2016

brrd commented Nov 3, 2016

cicithesquirrel commented Nov 4, 2016

darahak commented Nov 4, 2016

brrd commented Nov 5, 2016

cicithesquirrel commented Nov 7, 2016 •

edited

Loading

brrd commented Nov 8, 2016

brrd left a comment

brrd Nov 26, 2016

brrd Nov 26, 2016

brrd Nov 26, 2016

brrd Nov 26, 2016

brrd Nov 26, 2016

brrd Nov 26, 2016

brrd Nov 26, 2016

cicithesquirrel commented Dec 1, 2016

brrd Dec 1, 2016

cicithesquirrel Dec 1, 2016

nettnikl commented Nov 10, 2020

brrd commented Nov 11, 2020

Recent docs #92

Recent docs #92

Conversation

cicithesquirrel commented Jul 10, 2016

cicithesquirrel commented Jul 10, 2016

brrd commented Jul 10, 2016

cicithesquirrel commented Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

brrd commented Nov 3, 2016

cicithesquirrel commented Nov 4, 2016

darahak commented Nov 4, 2016

brrd commented Nov 5, 2016

cicithesquirrel commented Nov 7, 2016 • edited Loading

brrd commented Nov 8, 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

cicithesquirrel commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nettnikl commented Nov 10, 2020

brrd commented Nov 11, 2020

cicithesquirrel commented Jul 11, 2016 •

edited

Loading

cicithesquirrel commented Nov 7, 2016 •

edited

Loading