Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop: Add new setting to show note counts for folders and tags #2006

Merged
merged 11 commits into from
Nov 11, 2019

Conversation

erdody
Copy link
Contributor

@erdody erdody commented Oct 18, 2019

First stab at adding the ability to show note counts for Notebooks and Tags.
There's one menu item to toggle the feature.

  • Currently I'm calculating the immediate counts for folders, not the total for all the descendants. I can change it if people find the latter more intuitive
  • I was planning to add support to sort tags by count in a separate PR

Thanks.

@tessus tessus added the desktop All desktop platforms label Oct 18, 2019
@tessus
Copy link
Collaborator

tessus commented Oct 18, 2019

Currently I'm calculating the immediate counts for folders, not the total for all the descendants. I can change it if people find the latter more intuitive

I would make it dependent on the collapsed status of folders. The current calculation is a bit confusing when a folder is collapsed.

Calculate the sum of descendants + the folder itself, iff folder is collapsed:

- Folder A                  2 notes
     - Folder A.1           4 notes
           Folder A.1.1     1 note
           Folder A.1.2     3 notes
       Folder A.2           5 notes

- Folder A                  2 notes
     + Folder A.1           8 notes
       Folder A.2           5 notes

+ Folder A                 15 notes

But maybe this is just a dumb idea. In any case, I'm very happy that someone is looking into this again.

As for the count position: it should be right aligned. But that's just interface stuff.

Oh, and I would move the setting to Appearance, because the setting has nothing to do with the note, but rather the appearance of the UI. On the other side, Laurent might ask you why you created a setting in the first place.

Great job so far. Let's wait for Laurent's input.

@erdody
Copy link
Contributor Author

erdody commented Oct 18, 2019

Thanks for the feedback @tessus

I would make it dependent on the collapsed status of folders. The current calculation is a bit confusing when a folder is collapsed.

Yeah, you're right, but I also find it confusing that the number changes based on display options :/
Maybe I should just include descendants (independently of whether the folder is collapsed or not)

As for the count position: it should be right aligned. But that's just interface stuff.

I'm far from a css expert, but I gave it a try. Not sure how much value is the "notes" word adding, I would just leave the number.

Oh, and I would move the setting to Appearance, because the setting has nothing to do with the note, but rather the appearance of the UI. On the other side, Laurent might ask you why you created a setting in the first place.

I was just copying what "Show completed to-dos" toggle was doing, I didn't realize they show up in the Settings panel. Apparently if you don't specify a "section", it doesn't (for example, folders.sortOrder.field does that), so I went with that.

Thanks.

@erdody
Copy link
Contributor Author

erdody commented Oct 18, 2019

BTW, I just changed the folder note count calculation to always include descendants.
Let me know what you think.

@tessus
Copy link
Collaborator

tessus commented Oct 19, 2019

Yeah, you're right, but I also find it confusing that the number changes based on display options :/
Maybe I should just include descendants (independently of whether the folder is collapsed or not)

I think we have to look at how other apps are calculating items. The system I described seemed the most logical, but I could be wrong.

I'm far from a css expert, but I gave it a try. Not sure how much value is the "notes" word adding, I would just leave the number.

No, the notes text was just explanatory. In reality, there would be those little bezels/ovals with gray background and a number in them. But we can fix the visual stuff later. I'm not a CSS person either, but I'm sure we'll find someone who can help out with making this look great.

BTW, I just changed the folder note count calculation to always include descendants.
Let me know what you think.

Hmm, I don't see anything at all with the current code. No numbers that is.
But with the new calculation, it would look like this, which is even more confusing:

- Folder A                  15 notes
     - Folder A.1           8 notes
           Folder A.1.1     1 note
           Folder A.1.2     3 notes
       Folder A.2           5 notes

@erdody
Copy link
Contributor Author

erdody commented Oct 19, 2019

I couldn't easily find a similar application that has nested folders with counts.
The categories I see are:

  • No nesting supported (Boostnote/Simplenote)
  • Nesting but no counts (OSX Notes). Evernote has a notebook grouping concept called stack, but counts are not shown for them
  • Old file managers which only showed how many "items" each folder has (which includes direct children, folders and files).

So, the options I see so far are (in no particular order):
A. tessus count :) - show direct children if open but overall total if folded.
B. Always direct children. This one has the advantage that the number you see is the number of notes you'll get in the note list panel when you click on it.
C. Always overall notes. You will always see how many overall notes a given top level folder has, independently of the folding state.
D. No counts for folders.
E. Only counts for top level folders
F. "a la file system", similar to B but including direct folders in the count.
G. Show B and C, at the same time (two numbers per folder)

I find C more intuitive, but I don't have a strong preference.

@tessus
Copy link
Collaborator

tessus commented Oct 19, 2019

Ah, now I know where I have seen the tessus count (brilliant name ;-)) before: Thunderbird counts this way. e.g. for unread mails.

But I believe I have seen this in other apps as well. The thing is I do believe it is the most accurate and intuitive way to count.

If not, you'd have to go with option G, otherwise one type of info is just not available. e.g. With C you will never know how many notes are in a top folder (or any folder that has subfolders). With B you'll never know how many notes are under a folder.
With option A all info is available to the user. That is the reason why A or G are actually the only 2 viable options. Otherwise you wouldn't need a count in the first place, if you had to manually count notes to get the missing info.

But Laurent still hasn't voiced his opinion. @laurent22 what's your take on this?

@laurent22
Copy link
Owner

@erdody, I haven't had the opportunity to check the code in details yet but will do so as soon as possible.

@laurent22
Copy link
Owner

laurent22 commented Oct 31, 2019

I've just noticed the following issues:

  • Count for notebooks is not updated when adding or deleting a note. I don't think it should be updated in real time either in case multiple notes get deleted in one go. Instead it should be triggered by a timer (see the scheduleSave() method of NoteText.jsx for example).
  • Likewise, count for tags is not updated when adding or deleting a note
  • Any reason why there's a view to calculate the tags note count, but not the folders note count? I'd expect it would speed things up a bit?
  • The way the count is displayed is a bit heavy, I think it should just be a single number with a background colour to set it aside (make sure the background works in both light and dark themes).. On second thought, let's make it look like Evernotes - just a number in brackets with a faded colour.

image

  • If I click on the note count, it does nothing - the note count element should let the mouse clicks go through so that the folder/tag can still be selected through it (using CSS "user-select: none" should do the job)
  • I'd move the setting "showNoteCounts" to the "note" section as it's not really about how the app look but more like an extra feature. Please enable it by default.

For the count, I think the way it's done now is fine. Another good option would be the Thunderbird way I guess, but it will be trickier to implement.

@tessus
Copy link
Collaborator

tessus commented Oct 31, 2019

@laurent22

I'd move the setting "showNoteCounts" to the "note" section as it's not really about how the app look but more like an extra feature. Please enable it by default.

I don't understand this sentence. Actually it has exactly to do with how the app looks like. Without the setting the app shows a number next to a folder. Without it, it does not. It changes the appearance of the app.

I advised the OP to move it to Appearance, because this setting has to do with the appearance of app. This setting has nothing to do with the note. The note tab has settings that are related to a singular note. e.g. something when you open or edit a note. The count has nothing to do with this.
In fact the count would rather fit into a panel like Notebook or Appearance. But it does not belong to Note.

Here are some examples how note counts are used in other UIs (right aligned):

Screen Shot 2019-10-19 at 02 26 05

Screen Shot 2019-10-19 at 02 27 20

There's also the matter how notes should be counted. I think that's the most important part of this PR. e.g. I would find it unusable if they were counted any other way as I described in my first reply. (As mentioned in another reply, any other way would be missing one part of the info.)

@laurent22
Copy link
Owner

Ok let's go for the Appearance section then. For the note counts, I'm fine with either the number with a background, as in your example, as long as it works with all the themes without having to modify all them (i.e. by using one of the existing colour for the background). Or the Evernote way, with just plain text with faded background (theme.colorFaded) is fine too.

@tessus
Copy link
Collaborator

tessus commented Nov 1, 2019

The style of how the numbers are presented (or shown) can be easily chaged at any time. I'm more concerned about how notes are actually counted. The OP and I had a few back and forth arguments in this PR, so what do you think? What's you take on the counting algorithm?

@laurent22
Copy link
Owner

I've answered above, but let's wait for @erdody's take on all this.

@tessus
Copy link
Collaborator

tessus commented Nov 2, 2019

Ah, I missed the part:

For the count, I think the way it's done now is fine.

Ok, maybe in the future we can add an option how to count. That would be my favorite choice. In that case people can choose the way how notes are counted. I'm not insisting on this, it's just an idea for the future.

@tessus
Copy link
Collaborator

tessus commented Nov 2, 2019

e.g. anything other than A and G are useless to me. But that's a personal thing. ok, time for bed. ttyl.

@erdody
Copy link
Contributor Author

erdody commented Nov 4, 2019

@laurent22 @tessus Thanks for the feedback, I'm glad this is moving forward.

Let me try to summarize the pending items:

  • Style for counts

I just pushed a fix to make it look like Evernote
image

  • If I click on the note count, it does nothing

Fix pushed.

  • Count for tags/folders are not updated when adding/removing a note.

This seems to be working when adding a note, but I could verify that counts are not updated when removing a note. I will take a deeper look sometime this week.

  • Any reason why there's a view to calculate the tags note count, but not the folders note count? I'd expect it would speed things up a bit?

Yes, tags were already doing an outer join (to only show tags with notes), so it was safe (performance wise) to add a field for the counts. I just added a view to clean up multiple places where that query was needed.
Folders are not currently doing the outer join so I didn't want to add a performance penalty when you didn't have counts enabled.

@erdody
Copy link
Contributor Author

erdody commented Nov 4, 2019

@laurent22 I just pushed a fix for the folder/tag count update on note delete/add.

@tessus
Copy link
Collaborator

tessus commented Nov 4, 2019

@erdody I'm not sure what you did, but I think you merged master into your branch. This is a no-no.
You can rebase, but merging master is a bad idea. Why did you do this?

@erdody
Copy link
Contributor Author

erdody commented Nov 4, 2019

@tessus I believe @laurent22 was the one that merged master into my branch. See here:
image

I just merged my local branch with his commit because I didn't realize he committed something.

By the way, why is syncing with master a "bad idea"?

@tessus
Copy link
Collaborator

tessus commented Nov 4, 2019

By the way, why is syncing with master a "bad idea"?

I've experienced unforseen things happen. e.g. imagine 2 branches: In branch A, somebody makes a change to file X (X'). In branch B, somebody changes some other files, and pulls changes from master including file X (the original from master). Now after merging branch A and B, I've seen it happen that master had file X and not file X'.
Basically I have ssen that changes were reverted without anybody noticing. This doesn't have to be the case here or in this project, but I usually try to avoid things like that.

Anyway, Laurent will merge this, so I guess I'm off the hook. ;-)

@erdody
Copy link
Contributor Author

erdody commented Nov 5, 2019

@tessus I don't see why that should be the case, git should be able to identify the latest change in that scenario. In general, if the code has been pushed to the public (like in this case), I would avoid rewriting history (which is what rebase does).
You could argue that I could have rebased to merge with @laurent22 's commit (because my commit was not pushed when I did it), but the only advantage in that case is a prettier commit history... not worth the extra 2 or 3 commands, in my opinion.

@tessus
Copy link
Collaborator

tessus commented Nov 5, 2019

@erdody I agree, but I've seen strange things in certain cases. Maybe there was a bug in git or maybe somebody force pushed eiher master or a branch (I can't really recall), but I have seen it happen. Anyway, don't worry about it. All good. It's just me being too cautious.

@laurent22
Copy link
Owner

The merge commit happened after I fixed a conflict, but normally it should be fine. Thanks for the update @erdody, I'm going to take a look at it as soon as possible.

@tessus
Copy link
Collaborator

tessus commented Nov 5, 2019

@laurent22 I hope it will still be ok to add the Thunderbird count option in the future....

@@ -431,6 +435,11 @@ class SideBarComponent extends React.Component {
return this.anchorItemRefs[type][id];
}

noteCountElement(baseStyle, count) {
Copy link
Owner

Choose a reason for hiding this comment

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

baseStyle is not used, so can be removed.

@@ -431,6 +435,11 @@ class SideBarComponent extends React.Component {
return this.anchorItemRefs[type][id];
}

noteCountElement(baseStyle, count) {
let style = Object.assign({}, this.style().noteCount);
return <div style={style}>({count})</div>;
Copy link
Owner

Choose a reason for hiding this comment

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

There's no need for the intermediate style variable.

@@ -20,7 +20,12 @@ const reduxSharedMiddleware = async function(store, next, action) {
ResourceFetcher.instance().autoAddResources();
}

if (action.type == 'NOTE_DELETE') {
console.info(`reduxSharedMiddleware ${action.type}`);
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the console statement.

@laurent22
Copy link
Owner

Thanks @erdody, that's a solid PR and it's great that you've added tests for all the backend features. I've just left a few comments for minor issues, and once it's fixed we can merge.

@erdody
Copy link
Contributor Author

erdody commented Nov 9, 2019

Thanks @laurent22, all comments addressed.

@laurent22 laurent22 merged commit 9c98fb5 into laurent22:master Nov 11, 2019
@tessus
Copy link
Collaborator

tessus commented Nov 11, 2019

Great job. Just wanted to ask, if it was on purpose to put the setting in the section General and not in Appearance. Hmm, it also fits there, so never mind.

Well @erdody, if you're ever bored, you could always implement the Thunderbird count as an option. ;-) Hahaha.

@laurent22
Copy link
Owner

I don't have a strong opinion about where the setting should so in "General" section it's fine for me. For the Thunderbird count, if I understand correctly the difference is that we don't show the count if a notebook is expanded, is that correct?

@tessus
Copy link
Collaborator

tessus commented Nov 11, 2019

It's easier with an example:

Thunderbird count

- Folder A                  2 notes
     - Folder A.1           4 notes
           Folder A.1.1     1 note
           Folder A.1.2     3 notes
       Folder A.2           5 notes

- Folder A                  2 notes
     + Folder A.1           8 notes
       Folder A.2           5 notes

+ Folder A                 15 notes

current count

- Folder A                  15 notes
     - Folder A.1           8 notes
           Folder A.1.1     1 note
           Folder A.1.2     3 notes
       Folder A.2           5 notes

- Folder A                  15 notes
     - Folder A.1           8 notes
       Folder A.2           5 notes

+ Folder A                 15 notes

So the TB count shows the count aggregated when collapsed and the correct number of notes for the folder, if not collapsed.

e.g. with the current count you do not know how many notes are in folder A, unles you count them manually.

I don't have a strong opinion about where the setting should so in "General" section it's fine for me.

Same here. It also works in General. I just didn't expect it there, that's all.

@laurent22
Copy link
Owner

Ok that makes, so yes I'd accept a PR for a Thunderbird count, but no need to make it optional, it would just replace the current one.

@foxmask
Copy link
Contributor

foxmask commented Nov 11, 2019

thanks for this feature!

@flowli
Copy link

flowli commented Nov 11, 2019

I was totally thinking this is needed and then it arrived shortly after - thank you guys :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants