Desktop: Implement "show all notes"

[miciasto]

Resolves #262 for desktop

• Implement feature
• Implement + run tests
• Complete PR write up
• Fix automated test interaction

Description

• All notes is a new selectable section at the top of the sidebar.
• Selecting All notes updates the note list to show all notes.
• When navigating to All notes, the currently selected note remains selected.

While in All notes:

• All note list functions are enabled as usual (eg create, delete, add tags, etc)
• Notes are sorted according to the Sort notes by user preference.
• The note toolbar shows the notebook to which a note belongs
• New notes are assigned to the last selected notebook

Example showing behaviour when All notes is selected:

Design

Implemented as a smart filter. The PR also proposes the introduction of automated integration testing for the backbone.

Tests

• Unit tests for the library additions

Manual tests:

• Check all notes are shown from multiple notebooks and sub-notebooks
• Check notes are sorted correctly
• Check the note toolbar shows its containing notebook
• Check notes are created and deleted correctly
• Check the selected note maintains selection when navigating to the All notes

Comments

This implementation does not deal with requests for showing sub-notebook notes when a notebook is selected.

Doesn't show note count. Future enhancement if required.

[tessus]

I haven't tested the PR yet, but how are the notes sorted? According to the SQL statement they are not sorted at all. Or is this done in the UI anyway?
Does it respect the Sort note by setting?

[miciasto]

I haven't tested the PR yet, but how are the notes sorted? According to the SQL statement they are not sorted at all. Or is this done in the UI anyway?
Does it respect the Sort note by setting?

The sorting respects the Sort note by setting. It works exactly the same as for notebooks and tags, and in fact makes use the same mechanism that they already share.

[tessus]

@mic704b thx for clarifying this for me

[miciasto]

@tessus you might check the discussion in the github issue, I'd be interested in your opinion too on how we should handle creating notes when viewing all notes.

[miciasto]

Ready for initial review. Tests to be added.

Edit: Apologies, late update submitted.

[laurent22]

The design introduces a concept of a "View" or "virtual folder". A view is like folder that shows a filtered list of notes, but is not their parent. The "show all notes" is a most simple "view" that simply shows all notes.

In the codebase, there's already a concept of "smart filter", which is the same as your "View" concept. It's designed in such a way that any number of smart filters with various rules can be created, including by the user, and the app will display the relevant notes.

This concept is only used at the moment to display all the notes in the mobile app, so the code is simplified. Basically if a smart filter is specified, it's assumed it's the "all notes" one, so all notes are displayed without checking anything else.

I think you should be able to use the same concept for the desktop app. I'd expect there's not even that much code to change since the filter you need is exactly the same as for the mobile app. Basically you can simply check if (notesParentType === 'SmartFilter') { /* Display all notes */ }.

[miciasto]

Thanks for the explanation. I had seen the SmartFilter, but didn't realise the intention. Your explanation makes it clearer.

Ive now updated the PR attempting to use the SmartFilter. For the "all notes" smart filter, we use the same smart filter id as was nominated in the mobile app.

[miciasto]

@laurent22 I think I need to make the SmartFilter a model, ie a type of BaseItem, like Tag, Folder and Search, and assign it an id in the BaseModel.typeEnum_ .

Does that make sense? Is it compatible with your design?

In the mobile app, the SmartFilter is not a model, ie it does not derive from BaseItem or BaseModel. This works in the mobile app, because refreshNotes() only uses strings.

From notes.js refreshNotes():

		let notes = [];
		if (props.notesParentType === 'Folder') {
			notes = await Note.previews(props.selectedFolderId, options);
		} else if (props.notesParentType === 'Tag') {
			notes = await Tag.notes(props.selectedTagId, options);
		} else if (props.notesParentType === 'SmartFilter') {
			notes = await Note.previews(null, options);
		}

In the desktop app refreshNotes() works with BaseModel types. So to make it consistent, SmartFilter would be one too.

From BaseApplication.js refreshNotes():

		if (parentType === 'Folder') {
			parentId = state.selectedFolderId;
			parentType = BaseModel.TYPE_FOLDER;
		} else if (parentType === 'Tag') {
			parentId = state.selectedTagId;
			parentType = BaseModel.TYPE_TAG;
		} else if (parentType === 'Search') {
			parentId = state.selectedSearchId;
			parentType = BaseModel.TYPE_SEARCH;
		} else if (parentType === 'SmartFilter') {   // I propose this part
			parentId = state.selectedSmartFilterId;
			parentType = BaseModel.TYPE_SMART_FILTER;
		}

I don't want to leave the SmartFilter implementation completely skeleton and specific only to "show all notes", because have another feature in development that is another SmartFilter.

Please let me know, so I can fix this PR and complete the testing. It should be quick.

[laurent22]

@laurent22 I think I need to make the SmartFilter a model, ie a type of BaseItem, like Tag, Folder and Search, and assign it an id in the BaseModel.typeEnum_ .

Does that make sense? Is it compatible with your design?

Yes that makes sense and it was indeed the plan. So please go ahead and make it a model.

[miciasto]

The code and manual testing is complete and ready for review.

The automated testing is implemented and passes. However there is a side effect where the new tests are interfering with the initialisation of the existing automated tests. I am not sure what is going on so I have commented out the new unit tests temporarily until I can resolve it.

[miciasto]

@laurent22 Ready for review.

In completing this, I have introduced backbone "integration" tests.

// The integration tests are to test the integration of the core system, comprising the
// base application with middleware, reducer and models in response to dispatched events.
//
// The general strategy for each integration test is:
// - create a starting application state,
// - inject the event to be tested
// - check the resulting application state
//
// In particular, this file contains integration tests for smart filter features.

I may have abused the test framework in getting this to work, so please check it carefully. The way it works is to create a BaseApplication in the test harness which allows us to inject dispatch events to test the response. At the end of the tests, it is pulled down so it doesn't interfere with all the unit tests that follow.

Perhaps going forward, this pattern could be used to test a lot of the pre-existing functionality too.

If you are happy with it, I will expand the test cases for this feature (smart filter for show all notes) in a following PR, and maybe update development documentation to encourage others to use the strategy too.

Concerning the feature itself, I created a module Reserved as a sort register for reserved ids. To reduce decoupling, I didn't want to put them in their associated model (eg SmartFilter in this case) -- that could lead to circular dependencies and overcoupling.

[laurent22]

Yes I'm fine with the integration tests, I think they could be useful in particular to test what happens in the middleware.

I've noticed that you await baseApplication.dispatch, however dispatch normally is never async. You might have to wait after dispatch though (maybe with timeUtils.sleep?) because the middleware is async.

[miciasto]

I've noticed that you await baseApplication.dispatch, however dispatch normally is never async.

That was a mistake. Fixed now. Interesting that it made it work.

maybe with timeUtils.sleep

Yes, that works. Another way might be with a callback, but this is simpler even if a little less efficient.

[laurent22]

Perfect, it's good to merge then, thanks for the PR!

[miciasto]

Thank you!