GH-73: Implemented editor navigation history. #1649

kittaakos · 2018-04-04T13:18:53Z

Notes:

Currently, a location does not get removed when the corresponding file resource is deleted. That would require introducing @theia/filesystem in @theia/editor.
When an editor has multiple selections, the navigation service handles only the first content change.

Closes #73.

Signed-off-by: Akos Kitta [email protected]

epatpol · 2018-04-04T15:02:45Z

packages/editor/src/browser/navigation/navigation-location-service.ts

+    /**
+     * Checks whether the service can go [`forward`](#forward).
+     */
+    canGoFroward(): boolean {


Small typo! Froward -> Forward

akosyakov · 2018-04-08T08:11:41Z

packages/editor/src/browser/editor-navigation-contribution.ts

+    }
+
+    async onStart(): Promise<void> {
+        this.restoreState();


👍 We can do that; I left it out intentionally.

ok, i mostly was wondering whether it should have the same lifecycle as a layout. For example, in Atom there are hooks to store and restore data, we have it only for the layout. And Atom decides when it is a good time to store and restore. In our case it seems to be a responsibility of the extension developer. It could be bad in the long run for example if we change when the data should be stored on the shutdown then each extension should be updated. I will open an issue to discuss it separately.

In this case, would it make sense to store the state only onStop similar how we store the layout data?

Great idea, thanks! I was also wondering if we could do something like this in the FrontendApplication#startContributions:

await Promise.all(this.contributions.getContributions().filter(c => !!c.onStart).map(c => c.onStart!(this)));

Instead of starting each contribution one by one and wait for them (if it is a Promise). We just need to handle errors properly, and not fail fast.

Alright, let's leave as is.

In this case, would it make sense to store the state only onStop similar how we store the layout data?

It raises a question; FrontendApplicationContribution#onStop says: Note that this is implemented using `window.unload` which doesn't allow any asynchronous code anymore.. Still, we call async code from onStop, for instance: WorkspaceService. Do we still want to go with this approach? It does not seem to be safe.

In a nutshell: this.storageService.setData is async and we should not call it from window.unload.

We could have a sync API on the StorageService.

In a nutshell: this.storageService.setData is async and we should not call it from window.unload.

It is fine, it is implemented with LocalStorage which is synchronous. Maybe we should make StorageService synchronous as well. Let's keep the discussion in #1671. It is not in the scope of this PR.

akosyakov · 2018-04-09T08:17:00Z

Currently, a location does not get removed when the corresponding file resource is deleted. That would require introducing @theia/filesystem in @theia/editor.

We have a change event on a resource already. We can refine it to provide a change kind so clients can react on it, like mark an editor as deleted and remove a history location. Let's tackle it separately.

akosyakov · 2018-04-09T09:41:08Z

packages/editor/src/browser/editor-navigation-contribution.ts

+import { NavigationLocationService } from './navigation/navigation-location-service';
+
+@injectable()
+export class EditorNavigationContribution implements Disposable, MenuContribution, CommandContribution, KeybindingContribution, FrontendApplicationContribution {


We have EditorContribution. Would it make sense to merge this contribution into it? I think it is nice to have all constants for commands, menus provided by the extension in one place.

I don't mean to merge completely EditorContribution only registration of commands, menus and so on. Implementation can still live in something like EditorNavigationService and be called from EditorContribution.

What do you think?

I am OK with both ways. If you prefer moving the editor navigation contribution there, we can do it.

It is up to you. My main concern is to avoid spreading extension constants.

👍 Sure, as I said, I am OK with both, if you feel it is better, we can do it. It's a minor work.

akosyakov · 2018-04-09T09:59:26Z

I've tested it, not extensively, only for my cases. It works nicely for them. Code looks great as well.

kittaakos · 2018-04-09T11:13:07Z

TODOs before merge:

Fix typo.
Add await.
Move the navigation contribution to the editor contribution.
Rebase.
storing the state once on onStop lifecycle hook? - Anton

kittaakos · 2018-04-09T15:01:50Z

I have made a few changes based on your feedback, the review is obsolete. Could someone please approve it again? Thanks!

kittaakos · 2018-04-10T07:15:46Z

packages/editor/src/browser/navigation/navigation-location-service.ts

+     * Returns with the number of navigation locations that the application can handle and manage.
+     * When the number of locations exceeds this number, old locations will be erased.
+     */
+    maxStackItems(): number {


Just looked through the code again; this should be protected.

kittaakos · 2018-04-10T07:18:56Z

@simark, you have requested another review, shall we wait for that or are we good with this state?

simark · 2018-04-10T15:22:36Z

It was just because @MarkZ3 was actively waiting for this feature (if it is really what I think it is), so i thought he might want to give it a try. But if others are happy, go ahead, it's not like everything is set in stone after that.

kittaakos · 2018-04-10T15:31:45Z

OK. I have pushed a small change so the review became stale. I have reduced the implicit public visibility to protected. Others have already reviewed and approved it. @MarkZ3, if you have time, please try it out and approve it.

You should be able to navigate back with Ctrl+- and forward with Ctrl+Shift+-, just like in VSCode.

We ignore navigation location changes if those happen within +- 10 lines to the previous location. (Just like in VSCode).

MarkZ3 · 2018-04-10T15:44:25Z

@MarkZ3, if you have time, please try it out and approve it.

@kittaakos Sure, I'll give this a try in the next couple of hours.

MarkZ3 · 2018-04-10T17:29:37Z

The keyboard binding for "back" doesn't seem to work in Electron: when I do Ctrl+-, it zooms out (as it should??). I also don't see the command in the command palette. I typed "previous" and "back". I ended up noticing it in the main menu, under "Go", there I see that there is no keyboard mapping for back but there is for forward. Indeed, "forward" seems to work with the keyboard, but "back" only works through the menu. This is on Ubuntu BTW.
Probably the menu items should be greyed out when you cannot go forward or backwards anymore.

In the browser version, the menu items gray out properly and both shortcut are displayed in the menu. But similarly, the back keyboard binding zooms out.

Very excited to be able to use this BTW!

kittaakos · 2018-04-10T17:57:18Z

The keyboard binding for "back" doesn't seem to work in Electron: when I do Ctrl+-, it zooms out (as it should??).

I will verify this and will get rid of the keybinding collision on Ubuntu. Perhaps, the same problem present on Windows.

I also don't see the command in the command palette.

That is on purpose, but I can add it if you think it could be useful. Please do not take the current zoom in/out problem into account for now.

there is no keyboard mapping for back but there is for forward

That should be an electron bug; I will open a follow-up for that.

Probably the menu items should be greyed out when you cannot go forward or backwards anymore.

#446

Thanks for the review!

MarkZ3 · 2018-04-10T18:21:22Z

That is on purpose, but I can add it if you think it could be useful.

I think it makes sense. I see back/forward at the same level of other navigation commands like "go to definition". That's how I tried to find them at first. They are in the command palette in VS code too.

BTW, I would think only the "keybinding collision" is really blocking, the rest can be addressed in different patches/issues.

kittaakos · 2018-04-10T19:04:36Z

I think it makes sense.

Roger that. I will add it.

the "keybinding collision" is really blocking

I agree.

kittaakos · 2018-04-11T07:06:23Z

The command label should be qualified:

Go Back and
Go Forward

just like in VSCode.

kittaakos · 2018-04-11T12:52:19Z

We will go with Alt+← (to Go Back) and Alt+→ (to Go Forward) for now.

Closes #73. Signed-off-by: Akos Kitta <[email protected]>

kittaakos · 2018-04-11T13:29:42Z

I have verified on OS X (both on electron and browser) and on Windows too:

@MarkZ3, could you please try it out on Linux? Thanks!

MarkZ3 · 2018-04-11T14:49:44Z

@MarkZ3, could you please try it out on Linux? Thanks!

@kittaakos I just tested it and it seems to work well!

I'm personally not a big fan of how it works in VSCode because I think the history is a bit too detailed, and it seems to be similar here. But I would very much like to see this current proposal merged because it is very useful and later I can propose some tweaks or some preference.

kittaakos · 2018-04-11T15:08:00Z

history is a bit too detailed

We could make it configurable from the preferences. The implementation should be straightforward.

I would very much like to see this current proposal merged

Then please approve it, and I push the button ;)

MarkZ3 · 2018-04-11T15:18:11Z

I'm not 100% sure about the shortcut on macOS. Safari is option-leftarrow, VSCode is ctrl-cmd-leftarrow, and Xcode also. We can change later though.

MarkZ3 · 2018-04-11T15:19:39Z

Then please approve it, and I push the button ;)

I'll haven't checked the code yet but it shouldn't take long.

kittaakos · 2018-04-11T15:21:01Z

The code was reviewed and approved already twice. I have tested on OS X and Windows too. But it is your call.

MarkZ3 · 2018-04-11T15:22:31Z

Ah ok. Sounds good!

kittaakos · 2018-04-11T15:22:46Z

Furthermore, the browser's back/forward is bound to the Cmd+Left and Cmd+Right in both Chrome and Safari.

MarkZ3 · 2018-04-11T15:24:24Z

Furthermore, the browser's back/forward is bound to the Cmd+Left and Cmd+Right in both Chrome and Safari.

Woops, my bad. It is for me too. Got the keys mixed up :)

kittaakos requested a review from svenefftinge April 4, 2018 13:18

epatpol reviewed Apr 4, 2018

View reviewed changes

akosyakov reviewed Apr 8, 2018

View reviewed changes

akosyakov reviewed Apr 9, 2018

View reviewed changes

svenefftinge previously approved these changes Apr 9, 2018

View reviewed changes

akosyakov mentioned this pull request Apr 9, 2018

Serialization lifecycle hooks #1671

Open

kittaakos dismissed svenefftinge’s stale review via bae70a1 April 9, 2018 13:00

kittaakos force-pushed the GH-73 branch from 48931e5 to bae70a1 Compare April 9, 2018 13:00

simark requested a review from MarkZ3 April 9, 2018 22:24

akosyakov previously approved these changes Apr 10, 2018

View reviewed changes

kittaakos commented Apr 10, 2018

View reviewed changes

kittaakos dismissed akosyakov’s stale review via d3bdc3e April 10, 2018 15:24

kittaakos force-pushed the GH-73 branch from bae70a1 to d3bdc3e Compare April 10, 2018 15:24

GH-73: Implemented editor navigation history.

a33d86e

Closes #73. Signed-off-by: Akos Kitta <[email protected]>

kittaakos force-pushed the GH-73 branch from d3bdc3e to a33d86e Compare April 11, 2018 13:13

MarkZ3 approved these changes Apr 11, 2018

View reviewed changes

kittaakos merged commit 1d38488 into master Apr 11, 2018

kittaakos deleted the GH-73 branch April 11, 2018 15:23

kittaakos mentioned this pull request Apr 13, 2018

[editor-navigation] We have to check whether the file still exists during the restoreState #1702

Open

kittaakos mentioned this pull request Dec 13, 2018

[editor] update navigation-location stack by listening to filesystem changes #3822

Closed

GH-73: Implemented editor navigation history. #1649

GH-73: Implemented editor navigation history. #1649

Conversation

kittaakos commented Apr 4, 2018

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

akosyakov commented Apr 9, 2018

akosyakov Apr 9, 2018 • 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

akosyakov commented Apr 9, 2018

kittaakos commented Apr 9, 2018 • edited by paul-marechal Loading

kittaakos commented Apr 9, 2018

Choose a reason for hiding this comment

kittaakos commented Apr 10, 2018

simark commented Apr 10, 2018

kittaakos commented Apr 10, 2018

MarkZ3 commented Apr 10, 2018

MarkZ3 commented Apr 10, 2018

kittaakos commented Apr 10, 2018

MarkZ3 commented Apr 10, 2018

kittaakos commented Apr 10, 2018

kittaakos commented Apr 11, 2018

kittaakos commented Apr 11, 2018

kittaakos commented Apr 11, 2018

MarkZ3 commented Apr 11, 2018

kittaakos commented Apr 11, 2018

MarkZ3 commented Apr 11, 2018

MarkZ3 commented Apr 11, 2018

kittaakos commented Apr 11, 2018

MarkZ3 commented Apr 11, 2018

kittaakos commented Apr 11, 2018

MarkZ3 commented Apr 11, 2018

akosyakov Apr 9, 2018 •

edited

Loading

kittaakos commented Apr 9, 2018 •

edited by paul-marechal

Loading