Desktop: Added arrows to go front and back through note history #2563

naviji · 2020-02-23T18:28:35Z

Add forward and backward arrows to go through note history.

laurent22 · 2020-02-23T19:00:31Z

Note: we don't review incomplete PRs.

ghost · 2020-02-24T14:57:33Z

Hi @naviji and welcome to the Joplin-Community. I saw your PR in the forum and was interested.

So I just want to bring up my opinion about the position of the arrows. (I'm no designer nor developer) But I think the arrows shouldn't be placed there for some reason

In the viewer mode there is no need for a toolbar (there is also an issue for that) and without toolbar no going forwards and backwards in the viewer mode
this is a toolbar so I would expect clicking this button that some markdown magic is inserted

A suggestion could be - in my opinion - next to the search bar as this functions are somewhat related (I search for a note and want to go back to the previous)

Thanks anyway and the last word belongs to @laurent22 and @PackElend

Rishabh-malhotraa · 2020-02-26T18:09:29Z

@naviji I think you should squash your commits into fewer commits

naviji · 2020-02-26T18:31:49Z

@Rishgod Yes, I'll do that. But I want to wait until I get a review first.

naviji · 2020-02-26T19:01:57Z

@laurent22 I've implemented all the functionality requested in the issue. Please take a look.

laurent22 · 2020-02-26T23:52:07Z

It's a bit hard to follow the PR and I don't understand what "preHistoryNotes" means. History is always "previous" by definition. As you've noticed there's also already an "historyNotes" state so I'm concerned we might be duplicating state here. Could you explain your reasoning please?

naviji · 2020-02-27T04:40:55Z

Sorry. I should have named it better. My reasoning was that as you go backwards, the previous notes in historyNotes gets pushed into this stack so that you can go forward. Hence "prevHistoryNotes".

https://stackoverflow.com/questions/6869476/how-to-implement-back-and-forward-functionality-like-browser
historyNotes is the back stack and prevHistoryNotes is the next stack in this recipe. The two stacks have different uses. One keeps track of where to go back. And the other where to go forward. So there is no duplicate state.

Should I rename it like this?
historyNotes -> previousNotes
prevHistoryNotes -> nextNotes

laurent22 · 2020-02-27T18:19:40Z

Thanks for the explanation @naviji, it makes sense. Based on the link you've provided and explanation, please could you rename it like below?

historyNotes => backwardHistoryNotes
prevHistoryNotes => forwardHistoryNotes

Add forward and backward jumps through note list Note history updated when notes deleted Fixed note history when searching Rename historyNotes to backwardHistoryNotes, prevHistoryNotes to forwardHistoryNotes Removed invalid formatting

naviji · 2020-02-28T16:01:13Z

I did the renaming. I also did a rebase and squashed everything (Which maybe wasn't a good idea).
Should I add some tests or additional comments?

naviji · 2020-03-01T06:46:57Z

I've refactored the code. It should be more readable now. @laurent22

miciasto · 2020-03-03T09:00:40Z

Should I add some tests

Yes, before this feature can be merged, there should be full tests added.

For example,

For the reducer handleItemDelete and changeSelected folder, there should be unit tests added to the reducer.js unit test file.
For HistoryHelper there should be unit tests added to new services_HistoryHelper.js file. (Actually is this really a service? may be this will end up elsewhere)
For the changes to reducer function itself, and overall feature testing, there should be new integration feature tests created in integration_ForwardBackwardNoteHistory.js.

laurent22

Thanks for the update @naviji. The code is well written and I didn't notice any big issue. Could you also please add the tests as asked by @mic704b?

ReactNativeClient/lib/reducer.js

laurent22 · 2020-03-05T23:44:26Z

ReactNativeClient/lib/reducer.js

+
+		if (JSON.stringify(newState.selectedNoteIds) === JSON.stringify(noteIds)) return newState;
+


Could you explain the logic here please? For me it means "if the selection hasn't changed, return the modified state", which is no logical. If nothing has changed, we should return the same state (i.e. the state variable).

But maybe it's an incorrect copy/paste from the previous code and you have something else in mind.

It should just be return newState

JSON.stringify(newState.selectedNoteIds) === JSON.stringify(noteIds) will always be true since I am assigning newState.selectedNoteIds = noteIds before the if condition.

I removed if (JSON.stringify(newState.selectedNoteIds) === JSON.stringify(noteIds)) return state; because of this situation:

Suppose we are in note1. We then search for note1 again. Now when we press the back button,
JSON.stringify(newState.selectedNoteIds) === JSON.stringify(noteIds) will be true and the old state will be returned without doing the pop operation.

The history operations should be performed irrespective of JSON.stringify(newState.selectedNoteIds) === JSON.stringify(noteIds)

ReactNativeClient/lib/services/HistoryHelper.js

Add skeleton integration test

PackElend · 2020-03-15T09:31:30Z

There's only one case where it's "pop"

btw how can I understand pop, goto is clear, push can makes sense, how to explain pop?

laurent22 · 2020-03-15T09:37:55Z

Ok all good now, thanks @naviji!

…ough note history (#2563)" Fixing merge issue... This reverts commit d049b88.

naviji · 2020-03-15T12:34:13Z

There's only one case where it's "pop"

btw how can I understand pop, goto is clear, push can makes sense, how to explain pop?

@PackElend Look at this post.
push and pop refer to what action needs to take place on the backStack.

tessus · 2020-03-17T20:51:31Z

@naviji I haven't reviewed the code, but I was wondering what the limit was.

e.g. I have Joplin running for weeks before I install a new version and restart. What would happen, if I switched between 100,000 notes in that time? Just an exaggeration, but you'll see where I'm going with this. Will it really add 100,000 entries onto a stack? Also, is there some sort of optimization? e.g. if I switched between 2 notes for a few (N) times, would only be those 2 notes on the stack, or the notes * N switches?

miciasto · 2020-03-17T21:39:43Z

ReactNativeClient/lib/reducer.js

@@ -104,6 +105,20 @@ stateUtils.lastSelectedNoteIds = function(state) {
 	return output ? output : [];
 };

+stateUtils.getLastSeenNote = function(state) {


More generally, it gets the current note

miciasto · 2020-03-17T21:40:26Z

ReactNativeClient/lib/reducer.js

+			};
+		}
+	}
+};


Missing explicit return for when no note found

Good catch, and that makes me think I'll add an eslint rule for explicit returns as I believe there's one.

Sounds good. The eqeqeq rule might be worth a look too.

Yes, I didn't include it because when I started I didn't always use === and I'm concerned that changing everything to it now might create subtle bugs. New code should indeed use strict equality/inequality. Might still be worth reviewing at some point if we can enable it though.

So no explicit return statement is bad style. Got it.

miciasto · 2020-03-17T21:40:49Z

ReactNativeClient/lib/reducer.js

+
+			// Update history when searching


Missing test

tessus · 2020-03-17T21:41:35Z

@mic704b the code has already been merged

miciasto · 2020-03-17T21:43:39Z

@mic704b the code has already been merged

My mistake, I missed that.

miciasto · 2020-03-17T21:44:27Z

BTW there is also an issue if an un-selected note is deleted.

miciasto · 2020-03-17T21:48:18Z

@mic704b the code has already been merged

My mistake, I missed that.

It's strange it lets me enter new review comments after merge

laurent22 · 2020-03-17T22:49:25Z

Those are good points regardless. @naviji, please could you check Mic's comments and see if you can address them?

laurent22 · 2020-03-17T22:53:20Z

e.g. I have Joplin running for weeks before I install a new version and restart. What would happen, if I switched between 100,000 notes in that time?

I guess we could limit the history to 100 notes or so, what you think @naviji?

e.g. if I switched between 2 notes for a few (N) times, would only be those 2 notes on the stack, or the notes * N switches?

When I've tested I noticed that it would keep pushing the notes on the stack. That could be improved, for example by having a function that detects sequences in the history and remove successive duplicates, but it could be for another PR

tessus · 2020-03-17T23:01:52Z

I guess we could limit the history to 100 notes or so, what you think @naviji?

I think 1,000 would be fine too. Not that someone would really go back 1,000 notes. I only meant that there should be a limit. I actually don't really care what it is. For me 20 is probably enough, others might need more. 50, 100, 250. I'm fine with any number.

When I've tested I noticed that it would keep pushing the notes on the stack. That could be improved, for example by having a function that detects sequences in the history and remove successive duplicates, but it could be for another PR

A new PR is perfectly fine. It also allows for another good first issue. I just wanted to bring this up, that's all.

tessus · 2020-03-17T23:03:53Z

@mic704b

It's strange it lets me enter new review comments after merge

I think anything is fair game as long as the issue hasn't been locked. But even then collaborators can add/edit comments.

tessus · 2020-03-18T19:47:15Z

@naviji whatever you do, do not use this branch. Please work off a new branch from the latest git master

naviji · 2020-03-18T19:49:31Z

BTW there is also an issue if an un-selected note is deleted.

@mic704b How can I replicate it?

we could limit the history to 100 notes or so,

@laurent22 Firefox has a default value of 50 for its back button. I'd say 250 is more than enough.

if I switched between 2 notes for a few (N) times, would only be those 2 notes on the stack, or the notes * N switches?

having a function that detects sequences in the history and remove successive duplicates

@tessus Firefox and google chrome do no such optimization when clicking between two links repeatedly. I think it wouldn't be worth the trouble.

tessus · 2020-03-19T06:59:03Z

@naviji

I'd say 250 is more than enough.

Maybe we should use less then?

Firefox and google chrome do no such optimization when clicking between two links repeatedly. I think it wouldn't be worth the trouble.

Well, we have a slight problem though. Our situation is different than on browsers. As soon as a we switch to a different notebook, a note is always selected automatically. Therefore the list we build has potentially notes in there, which we actually do not want. But this is probably not something we can fix.
However, I think this PR caused another issue: #2803

miciasto · 2020-03-19T08:37:25Z

How can I replicate it?

Make a folder with some notes
Navigate through them to make some history
Select a note
Right-click another note in the list and delete it (but do not select it)
Use the back button to step backwards through the history

Observed:
When you navigate back over where the note was, the focus disappears completely, as if we have navigated to a hidden note.

Expected:
The navigation should skip the missing note and jump to the next on in the history.

naviji · 2020-03-20T08:24:55Z

@mic704b
I can't replicate it.

miciasto · 2020-03-22T00:04:05Z

I can't replicate it.

Neither can I anymore! I should have noted the commit number. Sorry.

steenstra · 2024-02-08T13:51:09Z

Is it possible to link these buttons to the back and forward buttons of my mouse?

naviji mentioned this pull request Feb 23, 2020

Add Back button when navigating between notes #2409

Closed

naviji requested a review from laurent22 February 23, 2020 18:33

PackElend added the gsoc-2020 label Feb 23, 2020

laurent22 added the WIP label Feb 23, 2020

laurent22 removed their request for review February 23, 2020 19:00

naviji changed the title ~~Desktop: Added arrows to go front and back through note links~~ Desktop: Added arrows to go front and back through note history Feb 24, 2020

naviji requested a review from laurent22 February 25, 2020 19:19

Rishabh-malhotraa mentioned this pull request Feb 28, 2020

[Feature Request] Navigation #716

Closed

Add front and back arrows for navigation

cc2543a

Add forward and backward jumps through note list Note history updated when notes deleted Fixed note history when searching Rename historyNotes to backwardHistoryNotes, prevHistoryNotes to forwardHistoryNotes Removed invalid formatting

Refactor code to make it more readable

a9e7c9a

laurent22 requested changes Mar 5, 2020

View reviewed changes

naviji added 8 commits March 6, 2020 15:04

Fix duplicate history

6beab1a

Moved getLastSeenNote to stateUtils

5bb6a2e

Merge branch 'master' of github.com:laurent22/joplin into jump-notes

4d99ce8

temp1

472d869

Move lastSeenNote finding to reducer

fd4e734

Implement unit tests

9b17c65

Merge branch 'master' of github.com:laurent22/joplin into jump-notes

aee1598

Add skeleton integration tests

f55e29b

Add skeleton integration test

Merge branch 'master' into jump-notes

9a9aeeb

laurent22 merged commit d049b88 into laurent22:master Mar 15, 2020

laurent22 added a commit that referenced this pull request Mar 15, 2020

Revert "Desktop: Reolves #2409: Added arrows to go front and back thr…

9e66219

…ough note history (#2563)" Fixing merge issue... This reverts commit d049b88.

naviji deleted the jump-notes branch March 15, 2020 14:32

miciasto reviewed Mar 17, 2020

View reviewed changes

ReactNativeClient/lib/reducer.js

Comment on lines +795 to +796

// Update history when searching

Copy link

Contributor

miciasto Mar 17, 2020

Choose a reason for hiding this comment

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

Missing test


		if (JSON.stringify(newState.selectedNoteIds) === JSON.stringify(noteIds)) return newState;

Desktop: Added arrows to go front and back through note history #2563

Desktop: Added arrows to go front and back through note history #2563

Conversation

naviji commented Feb 23, 2020 • edited Loading

laurent22 commented Feb 23, 2020

ghost commented Feb 24, 2020 • edited by ghost Loading

Rishabh-malhotraa commented Feb 26, 2020

naviji commented Feb 26, 2020

naviji commented Feb 26, 2020

laurent22 commented Feb 26, 2020

naviji commented Feb 27, 2020 • edited Loading

laurent22 commented Feb 27, 2020

naviji commented Feb 28, 2020 • edited Loading

naviji commented Mar 1, 2020 • edited Loading

miciasto commented Mar 3, 2020 • edited Loading

laurent22 left a comment

Choose a reason for hiding this comment

laurent22 Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

naviji Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

PackElend commented Mar 15, 2020

laurent22 commented Mar 15, 2020

naviji commented Mar 15, 2020 • edited Loading

tessus commented Mar 17, 2020

miciasto Mar 17, 2020

Choose a reason for hiding this comment

miciasto Mar 17, 2020

Choose a reason for hiding this comment

laurent22 Mar 17, 2020

Choose a reason for hiding this comment

miciasto Mar 18, 2020

Choose a reason for hiding this comment

laurent22 Mar 18, 2020

Choose a reason for hiding this comment

naviji Mar 18, 2020

Choose a reason for hiding this comment

miciasto Mar 17, 2020

Choose a reason for hiding this comment

tessus commented Mar 17, 2020

miciasto commented Mar 17, 2020

miciasto commented Mar 17, 2020

miciasto commented Mar 17, 2020

laurent22 commented Mar 17, 2020

laurent22 commented Mar 17, 2020

tessus commented Mar 17, 2020

tessus commented Mar 17, 2020

tessus commented Mar 18, 2020

naviji commented Mar 18, 2020

tessus commented Mar 19, 2020

miciasto commented Mar 19, 2020 • edited Loading

naviji commented Mar 20, 2020

miciasto commented Mar 22, 2020 • edited Loading

steenstra commented Feb 8, 2024

naviji commented Feb 23, 2020 •

edited

Loading

ghost commented Feb 24, 2020 •

edited by ghost

Loading

naviji commented Feb 27, 2020 •

edited

Loading

naviji commented Feb 28, 2020 •

edited

Loading

naviji commented Mar 1, 2020 •

edited

Loading

miciasto commented Mar 3, 2020 •

edited

Loading

laurent22 Mar 5, 2020 •

edited

Loading

naviji Mar 6, 2020 •

edited

Loading

naviji commented Mar 15, 2020 •

edited

Loading

miciasto commented Mar 19, 2020 •

edited

Loading

miciasto commented Mar 22, 2020 •

edited

Loading