Add note history (back/forward)

[naviji]

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

I would also like to change somethings:

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

Suggestions?

[tessus]

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]

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]

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]

Fair enough. What do you think @laurent22 ?

[laurent22]

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]

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]

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]

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]

@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]

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?

[naviji]

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]

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]

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

[miciasto]

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]

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]

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]

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]

@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]

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

[laurent22]

@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

[naviji]

Ok, ready for review

[naviji]

@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.

[laurent22]

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

[laurent22]

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]

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

[laurent22]

@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]

@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]

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

[laurent22]

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

[naviji]

@laurent22 Here is the rewritten test. #3328

[josh-wq]

Written test

[josh-wq]

Written test