Skip to content

Tags plugin #364

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

Closed
wants to merge 21 commits into from
Closed

Tags plugin #364

wants to merge 21 commits into from

Conversation

platers
Copy link
Contributor

@platers platers commented Jan 21, 2021

Adds tagging #249.

Very similar to marks, but non unique. Each tag has a tag root, which stores clones of all rows with that tag. Each tag root is marked for easy lookup.

'#' to create a tag.
'd#[number]' to delete a tag with index [number] (1 indexed)
'd#' to delete a tag if the row only has one tag

Cloning logic:
The tag root is the node with mark = tag
Rows are cloned when a tag is added to that row.
Rows are uncloned when a tag is deleted from that row.
If the tag root does not exist, one is generated at the document root and all tagged rows are cloned.
I'm not sure if this logic is best.

Should probably add unit tests

@WuTheFWasThat
Copy link
Owner

whoah, that's cool! I will try/look at it sometime soon. I didn't understand your notes about cloning logic though, could you explain?

@platers
Copy link
Contributor Author

platers commented Jan 21, 2021

the cloning logic is how the clones are maintained. Currently the clones are only updated when a tag is added or deleted from a row, or when the tag root is deleted.
This allows the user to mess around in the tag root, and the cloned rows might get out of sync with the actual rows with that tag. The only way to refresh the tag root is to delete it then tag a new row.
Also in this scheme, rows are not deleted from the tag root even if that row no longer exists elsewhere in the document.

I could imagine another scheme where the tag roots are refreshed all the time to make sure they are always in sync. Then the user wouldnt be able to change much in the tag root.

@WuTheFWasThat
Copy link
Owner

I see, if the tags are non-unique then I don't think they should share cloned children. if you want that functionality, you should instead just clone a row with marks and have them as children, i think?

@platers
Copy link
Contributor Author

platers commented Jan 21, 2021

hmm I'm not sure what you mean. I think the tags should share cloned children. If I tag a row #todo and #urgent I want that row to show up in both the #todo and #urgent roots.

@WuTheFWasThat
Copy link
Owner

ahh, i see. interesting! it's a cool feature, not what i had in mind

your logic soudns good. what happens if you add a child to the tag root? (seems fine if nothing happens)

@platers
Copy link
Contributor Author

platers commented Jan 21, 2021

yep nothing happens. My goal was to generalize #300. Now that I think about it, a tag search where you search for a tag and it returns the rows might be useful to for easier navigation. Not sure if that should be an additional feature or if it should replace cloning.

@WuTheFWasThat
Copy link
Owner

WuTheFWasThat commented Jan 21, 2021

yeah, the version of tags i had imagined in the past was:

  • you can tag rows with any number of tags
  • you can quickly search for everything with a given tag

however, your version is nice since searching isn't as good as seeing them all in one place

perhaps the basic version of tags can be a core feature of vimflowy, and then the extra feature of adding marks for each unique tag with cloned children could be a plugin (called "Tags To Mark" or something)

what do you think?

@platers
Copy link
Contributor Author

platers commented Jan 21, 2021

that sounds good to me! I'll work on that.

@platers
Copy link
Contributor Author

platers commented Jan 22, 2021

should be ready to try now. Search tags with '-'. Tag cloning is now another plugin.

@WuTheFWasThat
Copy link
Owner

awesome. will take a look over the weekend

Copy link
Owner

@WuTheFWasThat WuTheFWasThat left a comment

Choose a reason for hiding this comment

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

looks good, do you mind adding a couple tests also?

also there is a set of default plugins in datastore.ts where you can add tags!

@platers
Copy link
Contributor Author

platers commented Jan 24, 2021

addressed all your comments, let me know if you want more tests. Also I added a test-windows command in package.json to run unit tests on windows.

@WuTheFWasThat
Copy link
Owner

WuTheFWasThat commented Jan 24, 2021

tested briefly.

  • undo of deleting tags doesn't seem to work (probably good to add a test for that too :))
  • it might be nice if they looked different from marks, maybe they can be square without the rounded edges?
  • it could be nice to add some stuff to the default document, at src/assets/ts/configurations/vim.ts
  • nit: not a big deal if it's hard for you to get lint working, but there are many lint errors in clone_tags/index.tsx, mostly variables that were defined but never used

@platers
Copy link
Contributor Author

platers commented Jan 24, 2021

  • made tags square
  • added description to the default doc
  • for some reason lint isn't running on the clone tags plugin, I deleted as much as I can by eye
  • fixed undo for deleting tags. I had a bug where repeating d#1 with . froze the doc since the last tag would be deleted with just d#, leaving the 1 hanging. To fix this I removed the d# shortcut. Added tests for undo/redo/repeat.

found some bugs in tag clone, working on fixes

@WuTheFWasThat
Copy link
Owner

WuTheFWasThat commented Jan 24, 2021

nice!

that's too bad about the deleting tags... why does the hanging 1 freeze the doc? (most likely my fault, i just don't remember the code)

when i delete a tag then undo, it goes to a new position. however, redo still works - i guess because the mutation stored is by tag name and not index. this seems fine to me, seems a bit tricky overall and scary since undo doesn't quite restore the document to exactly the same state... not necessarily problematic right now though but it would be if there was also something that relied on tag indices

here's what i see for linting:
Screen Shot 2021-01-24 at 1 06 21 PM

@platers
Copy link
Contributor Author

platers commented Jan 24, 2021

  • made tags sorted
  • lots of bug fixes in tag clone
  • fix weird repeat behavior in tags
    Turns out the repeat freeze was because it was waiting to read a character forever. I fixed that, but couldn't figure out a nice way to implement the d# shortcut.

@WuTheFWasThat
Copy link
Owner

hm, so what happens if in the d# shortcut you just don't wait for a number if there's exactly one tag?

@platers
Copy link
Contributor Author

platers commented Jan 25, 2021

it made it freeze after repeating d#1 with '.' I'm not sure exactly why

@WuTheFWasThat
Copy link
Owner

ok I'll try it real quick

@WuTheFWasThat
Copy link
Owner

hmm it's a tricky bug in my code...

@WuTheFWasThat
Copy link
Owner

i pushed a fix to master, so something like this should no longer cause hangs
Screen Shot 2021-01-24 at 6 36 13 PM

@platers
Copy link
Contributor Author

platers commented Jan 25, 2021

nice! The shortcut seems to work now but some of the unit tests now fail. The 'can repeat' test gives 'Error: Queue stopped: queue is stopped!!'. Is there a difference between real use and sendKeys?

@WuTheFWasThat
Copy link
Owner

it seems to pass for me, could you push the latest tests to your branch?l

@platers
Copy link
Contributor Author

platers commented Jan 25, 2021

I just added the shortcut, no changes to the tests.

@WuTheFWasThat
Copy link
Owner

on line 66, this needs to change:

t.sendKeys('kd#1');

to

t.sendKeys('kd#');

otherwise it's waiting for something after the 1

@platers
Copy link
Contributor Author

platers commented Jan 25, 2021

oh that makes sense. 'can repeat' seems to have a different issue. d#1. works when I try it manually, but the test case is giving the queue is stopped error.

@WuTheFWasThat
Copy link
Owner

you were right, there was a weird issue affecting unit tests but not the browser. it's fixed now on master. in addition to that, i had these changes, after which tests pass!
8f073e6

@WuTheFWasThat
Copy link
Owner

merged

@platers
Copy link
Contributor Author

platers commented Jan 26, 2021

woot! I'm still working on some bugs in clone tags (ctrl+r doesnt work) but the core tags plugin should be completely functional.

@WuTheFWasThat
Copy link
Owner

ah ok sounds good

@platers platers mentioned this pull request Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants