[CLOSED] Sequential navigation in edit history

[core-ai-bot]

Issue by swmitra
Monday Jun 05, 2017 at 05:33 GMT
Originally opened as adobe/brackets#13418

---

This feature is something which I missed a lot! Remembering most recently used (interesting?) cursor positions and navigating there manually is really cumbersome.

This module adds the feature of remembering cursors/selections across full editors inside a project root and aids the user by providing 'Navigate Back'(Alt+I) and 'Navigate Fwd'(Alt+Shift+I) using the captured navigation frames.

Ping@petetnt@ficristo for review.

---

swmitra included the following code: https://github.com/adobe/brackets/pull/13418/commits

[core-ai-bot]

Comment by swmitra
Monday Jun 05, 2017 at 08:01 GMT

---

@petetnt Thanks a lot for reviewing this PR. It was really quick 👍
I have updated the code with relevant changes pertaining to review comments. All yours now 😄

[core-ai-bot]

Comment by ficristo
Monday Jun 12, 2017 at 16:55 GMT

---

I will not review, but I have a question.
If I understand correctly the navigation history is never emptyed but when you change project.
Doesn't it risk to consume too much memory? Usually this kind of things have a limit, like remembering only the last 100 navigation changes.

[core-ai-bot]

Comment by swmitra
Monday Jun 19, 2017 at 04:32 GMT

---

@ficristo You are right 👍 .
I had now put a limit of 30 and control overflow beyond that as having infinite navigation frames won't help anyone. As a developer even to do 30 back navigation would be too much.

[core-ai-bot]

Comment by swmitra
Monday Jun 19, 2017 at 04:35 GMT

---

@petetnt Can you please have a look at the PR again and if possible use it.
@MarcelGarber Can you please have a look at this PR?

[core-ai-bot]

Comment by swmitra
Monday Jun 19, 2017 at 10:36 GMT

---

@nethip Can you please have a look at this PR?

[core-ai-bot]

Comment by swmitra
Monday Jun 19, 2017 at 11:20 GMT

---

@petetnt Done. Fixed the last 2 nits and mixed tabs & spaces.

[core-ai-bot]

Comment by swmitra
Thursday Jun 22, 2017 at 05:14 GMT

---

@nethip Can you please have a look at this PR again?

[core-ai-bot]

Comment by nethip
Thursday Jun 22, 2017 at 10:37 GMT

---

@swmitra I think the code part is fine. But here are few questions I had

• What will happen if the file, for which a NavigationFrame is present in your JumpToStack or JumpedPosStack, is changed externally ?

• I am trying to understand why you have to use beforeSelection instead of selection changed? Had it been capturing after selection change, you won't have to rely on activePosNotSynced variable.

• I see that you have used currentFullEditor for switching between different files to restore a frame state. Does that mean selections captured for editors inside Inline editors will transform to full view when back or front command is fired?

• I see that you are relying on cm.setBookMark. What happens when any extension overrides the selection marks set, while selection being changed?

• Also for batch operations like Replace All, are you going to log all of the selection changes?

Edited:

• What happens if the selection was captured in a document from pane 1 and now the document is moved to pane 2?

[core-ai-bot]

Comment by swmitra
Thursday Jun 22, 2017 at 11:12 GMT

---

@nethip Thanks a ton for reviewing the PR 👍

I will try to answer the questions, please let me know if you think anything in the assumptions can go wrong.

• If we categorize external changes, we can see there are broadly 2 different kind of mutations. First one is the file entry itself being changed (delete/rename) and secondly the content being changed. In first case when we actually try to navigate using the stale frame, we end up discarding the frame as the entry doesn't exist on the disk. In second case, Brackets calls _resetText() on the existing editor with modified content which removes all the CM markers. So we gracefully discard the frame.(Let me verify it once again).

• I have used CM beforeSelection to capture the selection change "origin". This property is not part of selection change event as that's a simulated event by Brackets. We need to check for the origin of the change to discard frame capture in case user has used keyboard navigation keys or cursor change has happened because of character input. We just want to capture programmatic selection change (jump to def) or explicit pointer click.

• This part was getting really tricky to implement as showing the selection again in InlineEditor would be impossible for two reasons - First and the most prominent one is availability of "createInlineEditor" as a public API. I have seen multiple usage of this API even by extensions and then might show this in a different DOM container all together. That's the reason I listen only for full editors ( check for _paneId). So any navigation in inline editors are not captured at all.

• Once a textmark is created in CM, it can't be overridden. It can be removed though. In case it is removed , we won't be able to find it ("find()" will return empty position) and hence we will not act on it.

• Correct me if I am wrong, to highlight the matches in search/replace operation, we use CM text marker instead of actual selection and to actually replace the searched token we use replaceRange. So we don't use selection in these operations.

Please let me know if you think we can go wrong in any of these or other situations. Thanks again for raising this questions, I could verify lot of the scenarios after looking at this 😄

[core-ai-bot]

Comment by swmitra
Friday Jun 23, 2017 at 08:40 GMT

---

@nethip I have added additional handling for external changes. Also, deciding on which pane the document should open on navigation is determined based on the current master editors association with pane.

Please have a look at the additional changes.

[core-ai-bot]

Comment by swmitra
Tuesday Jun 27, 2017 at 09:26 GMT

---

@MarcelGerber Thanks a lot for reviewing this PR 👍
I have tried CM's implicit way to handle navigation but the problem was to handle navigation between multiple instances of CM which is the case we want to support predominantly. Then if we add handling of external changes and re initiating the edit session of the same file in a different CM instance, maintaining the marker cache inside CM was impossible. I had to externalize the whole thing to handle such scenarios.
Do let me know if you think it can be handled in a better way by supporting all the features what we are going to have with this implementation.

I have addressed most of the review comments. Please have a look at this PR and use it if possible.

[core-ai-bot]

Comment by schroef
Saturday Jul 08, 2017 at 20:25 GMT

---

Just downloaded the revision, but alt+i shows me this character ˆ??

OSX 10.11.6
Brackets 1.10

[core-ai-bot]

Comment by petetnt
Sunday Jul 09, 2017 at 00:22 GMT

---

Hi@schroef,

the OSX shortcuts were improved / fixed in adobe/brackets#13527, which didn't land to 1.10. You can change your shortcuts to match the ones in 13527 from Debug -> Show User Key Map and adding the following:

{
    "documentation": "https://github.com/adobe/brackets/wiki/User-Key-Bindings",
    "overrides": {
        "Cmd-Alt-Left": "navigation.jump.back",
        "Cmd-Alt-Right": "navigation.jump.fwd".
    }
}

[core-ai-bot]

Comment by schroef
Sunday Jul 09, 2017 at 01:40 GMT

---

Sorry but that doesnt do anything as well. Looking at that code, shouldn't that also be visible then in the navigation menu, if so it's not there in 1.10

[core-ai-bot]

Comment by schroef
Sunday Jul 09, 2017 at 01:43 GMT

---

Seems shortcuts work not normally like in other applications. I need to pres cmd+alt+ left or right in a sequence in order to let it do something. I seem to be able to jump through documents. It doesnt always jump back to the complete history of it locations though

[core-ai-bot]

Comment by swmitra
Sunday Jul 09, 2017 at 04:09 GMT

---

@schroef Brackets have a slightly different implementation for navigation. We don't remember all the positions rather only explicit positions. For example when you do mouse click at a position and pause for a brief moment to read something, do a jump to def or switch file to work on some other context.

[core-ai-bot]

Comment by swmitra
Sunday Jul 09, 2017 at 04:11 GMT

---

We are going to change the OSX shorcuts and provide a small UI place holder as well in the next pre release ( within a week's time!)

[core-ai-bot]

Comment by schroef
Sunday Jul 09, 2017 at 21:56 GMT

---

Well it doesnt store all the position i think. I've tried it couple times and sometimes is seems to work and sometimes not. I definitely dont see that history of 30 states.