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

Allow to remove all checked items (fix #1521) #1773

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicofrand
Copy link

Hi,

This PR solves the issue described in issue #1521 (ie. I want a quick action to clear the markdown from all checked items). It's quite simple but I am new to this language and environment so please tell if there are things that feel off.

Thanks in advance for the review, and thanks again for this useful app!

// remove them all together.
return matcher
.replaceAll("RANDOM-ISH_TEXT_TO_REPLACE")
.replaceAll("RANDOM-ISH_TEXT_TO_REPLACE\n?", "");
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a better idea, but this looks.... odd...

Copy link
Author

Choose a reason for hiding this comment

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

I know but I could not find a better way either

final Pattern pattern = Pattern.compile(
"^ *[-+*] \\[[xX]\\].*$",
Pattern.MULTILINE
);
Copy link
Member

Choose a reason for hiding this comment

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

This omits some variants that could lead to undefined behavior, for example checkboxes within code blocks. You can get some inspiration how to handle codeblocks in the nextcloud-commons library, but beware: It ain't that easy. Codeblocks can have three or more backticks, for example

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I will!

.concat("+ [ ] Line G")
.concat(newLine)
.concat(" + [ ] G1")
.concat(newLine);
Copy link
Member

Choose a reason for hiding this comment

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

As stated above, there are very many cases of the commonmark specification not considered (and also not covered by the tests)... @AndyScherzinger and @tobiasKaminsky will have to decide how compliant is "compliant enough" 😉

@stefan-niedermann
Copy link
Member

@AndyScherzinger you also maybe want to talk with @jancborchardt or @nimishavijay about the UX? The overflow menu is already quite crowded

@nimishavijay
Copy link
Member

Thanks for the contribution @nicofrand :) Could you please post a screenshot or video of the UI so that it is easier for everyone to review the design? :)

@nicofrand
Copy link
Author

Sure, here it is:

device-2023-05-19-154243.webm

@jancborchardt
Copy link
Member

jancborchardt commented May 25, 2023

Very nice contribution @nicofrand! :) I’m wondering if instead of a permanent action like this, it would be good to have one of either of these:

  • Automatically sort the checked items towards the bottom (of course within their hierarchy) → iOS Notes does this and it’s super useful e.g. for shopping lists and things like that, something you also describe in your issue. Then you do not need to create the list anew, you just uncheck it after you are done or ran out of the item.
  • The menu item could be a checkbox instead of a button, saying "Hide checked items"

What do you think @stefan-niedermann @nextcloud/designers?

@stefan-niedermann
Copy link
Member

We had this feature request a few times (hide or strike checked items, sort checked items to bottom), which I declined so far because

  • feature parity with the Notes server app¹
  • The scroll state of the preview and edit mode (which is restored when switching modes) would be different depending on the amount of checked items²
  • for sorting:
    • we didn't want to manipulate the content automatically in the background
    • we have no clear idea how to handle multiple lists separated by other elements

¹ Might be obsolete now when you implement it on server as well
² Maybe this should be considered for the rich editor as well

After all it's your decision now, and maybe some advanced handling of checkbox lists is indeed a good idea, but you should think about consequences, especially parity between the modes (view, edit, rich), parity with the ecosystem (notes server, notes iOS) and parity with other occurrences of the rich editor (files, deck, tables, polls, collectives, ...)

@nicofrand
Copy link
Author

Hi everyone!

So, if I sum up the concerns regarding this merge request, here are the points I see:

  1. This would also erase checked items inside code blocks. I think we could solve this by checking if the number of backticks from the beginning of the document until the beginning of the checkbox is impair (if there is 1 or 3 back ticks, we are in a code block, else we are not)?
  2. Design: maybe this should appear in another menu. It seemed appropriate for me to put this here, specially since there are no other menus but this can be discussed of course
  3. This should first be implemented in nextcloud notes itself. I am afraid I wouldn't have the time to do both :/. It would be great if an issue with all concerns solved first existed and just needed an implementation I guess.
  4. Replace the deletion with a hidden state. I don't see how that would help me as a user with a shopping list for ex. Moving items to the end would not reduce the size of the list. And what about duplicates then? Should the item be moved to the end of the document, end of the list, end of the sublist? I feel like this is a different feature…

Could you guys tell me if I should move forward to solve point 1 or if there is no current change of merging this?

@jancborchardt
Copy link
Member

I would say if it’s the simple hiding option like so:

The menu item could be a checkbox instead of a button, saying "Hide checked items"

It would be good, and it would also not necessarily also be implemented in the web app too.

@nicofrand what do you mean by it wouldn't help you with a shopping list? Usually shopping lists are similar across shopping runs, otherwise you have to type all deleted items again and again.

@stefan-niedermann what do you think?

@nicofrand
Copy link
Author

@nicofrand what do you mean by it wouldn't help you with a shopping list? Usually shopping lists are similar across shopping runs, otherwise you have to type all deleted items again and again.

Having it in a bottom list, usually with a stroke, often creates duplicates (I don't remember if I already added it, where in the list, as a plural or not, etc.).

@jancborchardt
Copy link
Member

Having it in a bottom list, usually with a stroke, often creates duplicates (I don't remember if I already added it, where in the list, as a plural or not, etc.).

With the hiding option I mean that the items should actually not be moved around in the list, just hide checked items. :) So a purely client-side view switch. Does that make sense?

@nicofrand
Copy link
Author

Ah ok, only in the client view! But that would not help when editing a list (cleaning all items that I bought, letting the ones missing)…

@AndyScherzinger
Copy link
Member

Rebased due to major technical updates in terms of libraries, especially AGP and SSO

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

Successfully merging this pull request may close these issues.

5 participants