Add note history (back/forward) #2819

naviji · 2020-03-20T15:03:42Z

I also edited the other tests with the fixes by @mic704b

I would also like to change somethings:

Rename the 'push' and 'pop' historyAction to 'goBackward' and 'goForward'.
Adding a history limit of 200.
Changing the position of the arrows. (Near the search bar as recommended by @nr458h)

Suggestions?

tessus · 2020-03-20T17:56:17Z

Changing the position of the arrows. (Near the search bar as recommended by @nr458h)

No, the arrows should stay where they are. Why would you want to move the mouse all over to the search bar. This makes no sense and is not very user friendly.

Unless you have a mockup that shows the difference and it turns out to be better, I'm against changing the position of the arrows.

naviji · 2020-03-20T18:40:11Z

I mostly agree with you.

Theses are the reasons @nr458h mentioned.

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)

Also when I see the new WYSIWYG toolbar, putting arrows there seems misplaced to me.

Finally, this looks weird

But I get that it's too far away to easily navigate between notes.

tessus · 2020-03-20T19:24:13Z

I don't care about the experimental WYSIWYG toolbar.

In the viewer mode there is no need for a toolbar

This is one person's opinion, and I'm very sorry to be so blunt, but a random person does not make design decisions.
Furthermore, in viewer mode, we still have the following icons:

naviji · 2020-03-20T20:16:13Z

Fair enough. What do you think @laurent22 ?

laurent22 · 2020-03-20T21:52:28Z

Yes I also think we should leave them where they are now, unless someone makes a convincing case to move them elsewhere.

I feel they should be completely to the left of the toolbar though, even before the "In: Notebook" button. What do you think?

laurent22 · 2020-03-20T21:54:31Z

Rename the 'push' and 'pop' historyAction to 'goBackward' and 'goForward'.

This is a very good idea, and it will make it consistent with the other action "goto".

Agree too with the history limit.

miciasto · 2020-03-22T00:37:55Z

One issue with the button location is the buttons move if you are using them in All Notes, and the navigation takes you to a folder. So if you are too quick, you end up unintentionally editing a note.

The buttons move because the In:FolderName element in the toolbar is removed when viewing the folder.

In All notes

In a folder:

If you press <- quickly to run back through the history, all of a sudden you find yourself unexpectedly pressing the bold or italics button and editing a note.

So keeping them in the left most position on the toolbar might be nicer.

Edit: OK, I now see Laurent has already suggested this above.

miciasto · 2020-03-22T00:40:40Z

I do notice a different issue though, the history doesn't seem to track keyboard navigation, ie when I move around the note list using the keyboard.

master commit 159eaf7

naviji · 2020-03-23T17:53:13Z

@mic704b Some integration tests in showAllNotes were still failing for me. I applied your fix and it works now. Replacing await time.msleep(100); with await testApp.wait(); is enough, isn't it?

~~Also could you please check why my travis osx test is not even starting?~~
Apparently package-lock.json files are important.

miciasto · 2020-03-23T20:17:00Z

Some integration tests in showAllNotes were still failing for me.

There is a waiting PR #2777 for this.

The travis problem is unrelated, it might have just been a temporary glitch.

miciasto

Thanks @naviji

I also have a general design question: what is the advantage of choosing to implement this feature as separate forward and backward history arrays, rather than a single history array and a pointer within it?

CliClient/tests/feature_NoteHistory.js

CliClient/tests/reducer.js

CliClient/tests/feature_NoteHistory.js

CliClient/tests/reducer.js

ReactNativeClient/lib/reducer.js

CliClient/tests/feature_NoteHistory.js

ReactNativeClient/lib/reducer.js

naviji · 2020-03-24T04:58:29Z

To implement backward and forward functionality as in browsers or file explorers we need two stacks.

Check this out.
https://stackoverflow.com/questions/6869476/how-to-implement-back-and-forward-functionality-like-browser

miciasto · 2020-03-24T12:46:47Z

With your integration test, you have two helper functions goBackward() and goForward()

const goBackWard = (state) => {
	const lastItem = state.backwardHistoryNotes[state.backwardHistoryNotes.length - 1];
	testApp.dispatch({ type: 'FOLDER_AND_NOTE_SELECT', noteId: lastItem.id, folderId: lastItem.parent_id, historyAction: 'goBackward' });
};

Is this intention of this function to simulate the user pressing the back button?

naviji · 2020-03-24T15:07:51Z

Yes.
goBackward -> press the back button
goForward -> press the forward button
goToNote -> click on a note

miciasto · 2020-03-24T21:00:37Z

Yes.
goBackward -> press the back button
goForward -> press the forward button
goToNote -> click on a note

Ok, good. We need to change it though, so it is like what the front end does.

Think of the feature test as if you asked a user to test the app for you. The state.backwardHistoryNotes is invisible to them, its important, but not visible. What they see is the currently selected folder and note. Now, we don't have a full front end support for the harness, so we are doing the best we can.

So the idea is to feed in the events exactly like what would come from the front end, and check that the visible response to the user is as expected. The visible response to the user for this feature is primarily the selected folder and note. So that's what we should be checking in the expect checks. The test actions need to be tweaked to be more like what comes from the front end. For example, I don't think the front end uses state.backwardHistoryNotes when generating its NOTE_SELECT event, so here, neither should we.

The unit tests are for testing the internal states, individual methods and low level details. The feature tests are trying to be more higher level, across the app, end to end, to make sure all the bits are working together right.

Also it might help to think of it like this. Imagine I decided to replace your implementation with a circular buffer. (I'm not going to, your implementation is good, this is just hypothetical!). If I ran your feature tests, they would all fail because my circular buffer doesn have a backwardsNoteHistory array. But if we write the feature test right, then I could run them on my new implementation without any change to them.

So, for context, the goal of the feature tests are to check the high level behaviour, end to end. And two reasons are to make sure your implementation works, and to make sure no one changes something else in the system and accidently breaks your feature.

naviji · 2020-03-25T04:35:12Z

Thanks @mic704b for the clear explanation.

I was writing integration tests like unit tests. I'll change my Expect statements to reflect the expectations of a user clicking the arrows.

I don't think the front end uses state.backwardHistoryNotes when generating its NOTE_SELECT event, so here, neither should we.

But it does. Here is the left arrow's code in the front end.

	tooltip: _('Back'),
	iconName: 'fa-arrow-left',
	enabled: (this.props.backwardHistoryNotes.length > 0),
	onClick: () => {
		if (!this.props.backwardHistoryNotes.length) return;
		const lastItem = this.props.backwardHistoryNotes[this.props.backwardHistoryNotes.length - 1];
		this.props.dispatch({
			type: 'FOLDER_AND_NOTE_SELECT',
			folderId: lastItem.parent_id,
			noteId: lastItem.id,
			historyAction: 'goBackward',
		});
	}

We are using backwardHistoryNotes for two things. First, to disable the button if there is no history.
Second, to get the id of the note and folder we should select when the button is pressed.

The NOTE_SELECT event uses the noteId attribute of this action.
Similarly the FOLDER_SELECT event uses the folderId.

I don't see how we can decouple the backwardHistoryNotes from the button click.
And the goBackward function is just a copy of this onClick event handler.

miciasto · 2020-03-25T19:22:43Z

And the goBackward function is just a copy of this onClick event handler.

I stand corrected on that bit! That's exactly what we want. Excellent!

I was writing integration tests like unit tests. I'll change my Expect statements to reflect the expectations of a user clicking the arrows.

Great! And I will update the documentation so this is clearer for people writing feature tests in the future. Thanks @naviji.

miciasto · 2020-03-28T00:49:53Z

Ok I think I figured out what I saw before. Can you reproduce these?

After selecting notes in one folder, selecting another folder seems to corrupt the history:

After selecting notes in a folder, moving a note to another folder seems to corrupt the history:

miciasto · 2020-04-20T10:22:23Z

@laurent22 perhaps it might be worthwhile to schedule in a review of #2777 in case it can be merged? It is only small but has some changes that affect this PR.

laurent22 · 2020-04-20T10:33:04Z

Yes, that's quite straightforward, I've merged it now.

laurent22 · 2020-04-20T10:36:16Z

@tessus Why not? Everywhere I looked it says committing them is recommended. What am I missing?

There's a bug in npm, which makes it change package-lock all the time even though nothing has changed. If we were merging it, there would be many unneeded commits on this file and probably frequent conflicts.

So we only merge when it's needed - i.e. when something has changed in package.json; or when running npm audit fix

…tory-test

naviji · 2020-04-22T13:38:10Z

Ok, ready for review

naviji · 2020-04-30T13:59:30Z

@laurent22 ~~Should we remember the cursor position too?~~ On second thought, this pull request is large enough as it is. But remembering the cursor position seems to be an enhancement that some people want. I'll take a look and see if I can do something about it.

Edit: It looks like the old feature_ForwardBackwardNoteHistory.js is causing everyone's test to fail!
Maybe you should delete it even before this gets merged.

This reverts commit d1cab4b. Part of this revert: d2582f4 For rationale see #2819 (comment)

laurent22 · 2020-05-02T15:52:16Z

I reverted this commit as it was linked to the history feature: ec8ccc9

ReactNativeClient/lib/reducer.js

laurent22 · 2020-05-08T16:18:18Z

Thanks for fixing the conflicts @naviji, I indeed expected there might be a few after the recent refactoring of the text editor.

I'm not entirely sure that we need combineReducer to simplify the code. Perhaps you could however move your history-specific functions to a separate file.

Also I don't think you need to call handleHistory a bit everywhere as you're doing now. You could just add newState = handleHistory(newState, action) once, at the bottom of the reducer, then in your handleHistory function you filter on the action type. Or maybe one call for pre-processing on top of the reducer, and one call for post-processing at the bottom.

That way there are minimal changes to reducer.js, and all the history logic will be in a separate file, which makes it easier to maintain long term.

laurent22 · 2020-05-15T11:14:45Z

Thanks @naviji for this very useful feature, and thanks @mic704b for reviewing and providing support!

laurent22 · 2020-06-01T00:02:41Z

@naviji, one of the note history tests is randomly failing on CI, for example on these two pull requests:

https://travis-ci.org/github/laurent22/joplin/jobs/693070952#L1005

https://travis-ci.org/github/laurent22/joplin/jobs/692433735#L1005

Failures:

1) integration_ForwardBackwardNoteHistory should ensure no adjacent duplicates

  Message:

    Expected $[0] = '432a1b51aed74df5b2f13cf9a77a492d' to equal '39fc72bd2fa64faa9b2adc424f5c2b06'.

  Stack:

    Error: Expected $[0] = '432a1b51aed74df5b2f13cf9a77a492d' to equal '39fc72bd2fa64faa9b2adc424f5c2b06'.

        at <Jasmine>

        at asyncTest (/home/travis/build/laurent22/joplin/CliClient/tests-build/feature_NoteHistory.js:239:33)

297 specs, 1 failure

Finished in 147.573 seconds

Randomized with seed 99866 (jasmine --random=true --seed=99866)

Do you know what could be the reason and how to fix it?

naviji · 2020-06-02T04:30:09Z

@laurent22 I don't know why it fails randomly but, now that I look at this test again, I can see that it will pass even if we don't remove adjacent duplicates.

I'll rewrite the test, and we'll see if that fixes it. Is that okay?

laurent22 · 2020-06-02T16:58:05Z

@naviji, yes that would be great if you could fix it. It's happening semi-regularly on pull requests.

laurent22 · 2020-06-04T17:27:33Z

For now I have disabled the tests, as it makes most new pull requests fail: afcfb0e

naviji · 2020-06-05T02:34:59Z

@laurent22 Here is the rewritten test. #3328

josh-wq · 2020-12-29T13:25:17Z

Written test

josh-wq

Written test

PackElend added the gsoc-2020 label Mar 21, 2020

naviji added 7 commits March 23, 2020 20:02

Add test for search in feature_NoteHistory

0d10505

Rename historyAction

5ea0f9d

Add history limit of 200

1c5bc1f

Save history on keydown

ad16d13

Remove duplicates in history due to deletion

7dcd255

Add test for adjacent duplicate deletion

eceaebc

Fix showAllNotes tests

1856638

Add package-lock.json files

b77e762

miciasto suggested changes Mar 23, 2020

View reviewed changes

naviji added 2 commits March 25, 2020 11:10

Clean up

9191be6

Rename lastSeenNote to currentNote

4504358

Update goBackWard, goForward functions

626d966

naviji added 4 commits April 21, 2020 07:36

Merge branch 'master' of https://github.com/laurent22/joplin into his…

566bea6

…tory-test

Remove package-lock.json

f625666

Remove old history test file

baed477

Clean up

bab891b

Fix search bug

49a4a30

laurent22 added a commit that referenced this pull request May 2, 2020

Revert "Tests: Fix random failures (#2777)"

ec8ccc9

This reverts commit d1cab4b. Part of this revert: d2582f4 For rationale see #2819 (comment)

naviji added 2 commits May 6, 2020 21:50

Merge from upstream master

4e00206

Add arrows to WYSIWYG editor

9972588

naviji commented May 8, 2020

View reviewed changes

ReactNativeClient/lib/reducer.js Outdated Show resolved Hide resolved

Simplify

ffc01e7

laurent22 merged commit b3f32ff into laurent22:master May 15, 2020

laurent22 mentioned this pull request May 26, 2020

[Feature Request] Navigation #716

Closed

naviji mentioned this pull request Jun 5, 2020

All: Rewrite randomly failing test #3328

Merged

josh-wq reviewed Dec 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add note history (back/forward) #2819

Add note history (back/forward) #2819

naviji commented Mar 20, 2020 •

edited

Loading

tessus commented Mar 20, 2020

naviji commented Mar 20, 2020

tessus commented Mar 20, 2020

naviji commented Mar 20, 2020

laurent22 commented Mar 20, 2020

laurent22 commented Mar 20, 2020

miciasto commented Mar 22, 2020 •

edited

Loading

miciasto commented Mar 22, 2020 •

edited

Loading

naviji commented Mar 23, 2020 •

edited

Loading

miciasto commented Mar 23, 2020

miciasto left a comment

naviji commented Mar 24, 2020

miciasto commented Mar 24, 2020

naviji commented Mar 24, 2020

miciasto commented Mar 24, 2020 •

edited

Loading

naviji commented Mar 25, 2020 •

edited

Loading

miciasto commented Mar 25, 2020 •

edited

Loading

miciasto commented Mar 28, 2020

miciasto commented Apr 20, 2020

laurent22 commented Apr 20, 2020

laurent22 commented Apr 20, 2020

naviji commented Apr 22, 2020

naviji commented Apr 30, 2020 •

edited

Loading

laurent22 commented May 2, 2020

laurent22 commented May 8, 2020 •

edited

Loading

laurent22 commented May 15, 2020

laurent22 commented Jun 1, 2020 •

edited

Loading

naviji commented Jun 2, 2020

laurent22 commented Jun 2, 2020

laurent22 commented Jun 4, 2020

naviji commented Jun 5, 2020

josh-wq commented Dec 29, 2020

josh-wq left a comment

Add note history (back/forward) #2819

Add note history (back/forward) #2819

Conversation

naviji commented Mar 20, 2020 • edited Loading

tessus commented Mar 20, 2020

naviji commented Mar 20, 2020

tessus commented Mar 20, 2020

naviji commented Mar 20, 2020

laurent22 commented Mar 20, 2020

laurent22 commented Mar 20, 2020

miciasto commented Mar 22, 2020 • edited Loading

miciasto commented Mar 22, 2020 • edited Loading

naviji commented Mar 23, 2020 • edited Loading

miciasto commented Mar 23, 2020

miciasto left a comment

Choose a reason for hiding this comment

naviji commented Mar 24, 2020

miciasto commented Mar 24, 2020

naviji commented Mar 24, 2020

miciasto commented Mar 24, 2020 • edited Loading

naviji commented Mar 25, 2020 • edited Loading

miciasto commented Mar 25, 2020 • edited Loading

miciasto commented Mar 28, 2020

miciasto commented Apr 20, 2020

laurent22 commented Apr 20, 2020

laurent22 commented Apr 20, 2020

naviji commented Apr 22, 2020

naviji commented Apr 30, 2020 • edited Loading

laurent22 commented May 2, 2020

laurent22 commented May 8, 2020 • edited Loading

laurent22 commented May 15, 2020

laurent22 commented Jun 1, 2020 • edited Loading

naviji commented Jun 2, 2020

laurent22 commented Jun 2, 2020

laurent22 commented Jun 4, 2020

naviji commented Jun 5, 2020

josh-wq commented Dec 29, 2020

josh-wq left a comment

Choose a reason for hiding this comment

naviji commented Mar 20, 2020 •

edited

Loading

miciasto commented Mar 22, 2020 •

edited

Loading

miciasto commented Mar 22, 2020 •

edited

Loading

naviji commented Mar 23, 2020 •

edited

Loading

miciasto commented Mar 24, 2020 •

edited

Loading

naviji commented Mar 25, 2020 •

edited

Loading

miciasto commented Mar 25, 2020 •

edited

Loading

naviji commented Apr 30, 2020 •

edited

Loading

laurent22 commented May 8, 2020 •

edited

Loading

laurent22 commented Jun 1, 2020 •

edited

Loading