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

Index tags #100

Merged
merged 4 commits into from
Jul 2, 2023
Merged

Index tags #100

merged 4 commits into from
Jul 2, 2023

Conversation

thombruce
Copy link
Owner

@thombruce thombruce commented Jul 2, 2023

closes #98


Internal use. Do not delete.

  • Tests passing
  • Coverage sufficient
  • Manual review
  • No A11y regression
  • Translations provided or not needed

@netlify
Copy link

netlify bot commented Jul 2, 2023

Deploy Preview for toodles ready!

Name Link
🔨 Latest commit 6dd58ab
🔍 Latest deploy log https://app.netlify.com/sites/toodles/deploys/64a1a77204b19e00086d95c4
😎 Deploy Preview https://deploy-preview-100--toodles.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

github-actions bot commented Jul 2, 2023

Coverage Summary for `./packages/web`

Status Category Percentage Covered / Total
🟢 Lines 67.68% / 60% 421 / 622
🟢 Statements 67.68% / 60% 421 / 622
🟢 Functions 60.97% / 60% 25 / 41
🟢 Branches 70.31% / 60% 45 / 64
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
packages/web/src/components/ContextTag.vue 20% 100% 0% 20% 4-15
packages/web/src/components/HashTag.vue 20% 100% 0% 20% 4-15
packages/web/src/components/ProgressBar.vue 70.17% 66.66% 100% 70.17% 16-23, 26-34
packages/web/src/components/ProjectTag.vue 20% 100% 0% 20% 4-15
packages/web/src/components/TagTag.vue 71.42% 100% 0% 71.42% 6-7
packages/web/src/components/TodoItem.vue 80.7% 40% 28.57% 80.7% 21, 26-27, 33-40
packages/web/src/components/TodoList.vue 73.68% 100% 100% 73.68% 11-15
packages/web/src/components/TodoPriority.vue 20% 100% 0% 20% 4-15
packages/web/src/components/TodoText.vue 61.7% 57.14% 85.71% 61.7% 15-18, 21-25, 30, 33-36, 39-42
packages/web/src/models/Todo.ts 90.8% 80% 100% 90.8% 35-40, 56-57
packages/web/src/plugins/dexie.ts 55.12% 100% 66.66% 55.12% 38-41, 45-55, 59-78
packages/web/src/plugins/lunr.ts 78.57% 100% 0% 78.57% 10-12
packages/web/src/plugins/timepiece.ts 71.42% 60% 50% 71.42% 14-19
packages/web/src/stores/todos.ts 72.57% 90% 83.33% 72.57% 21, 25, 29-31, 35-37, 41-43, 47-49, 53-55, 59-61, 65-67, 71-73, 77-79, 83-85, 89-91, 95-97, 103, 107-115

@thombruce
Copy link
Owner Author

thombruce commented Jul 2, 2023

  • See two todos in notes

At present, we're only doing this on todo creation. It must be done for update too.

We also aren't indexing tags because arrays of objects can't be indexed - neither can their individual keys (at least not with Dexie's indexes syntax). Consider indexing the keys separately in a new pseudo-attribute: tagKeys. Tag values need not be indexed? What about sorting? Good point - we do have intentions to sort against a hidden value for timers; it would be handy to have this indexed.

Really what tags need is a lot more thought, especially as pertains to making their values interactive. Being able to search by key is valuable. Being able to sort by value is also valuable. And in some cases, this value should be a hidden value (its format will differ from the one presented to the user).

I would say that probably means tags indexing should be handled at the point of introducing interactivity, so on #97. I'll link to here from there...

@thombruce
Copy link
Owner Author

Bit of a problem at the moment even updating this so that tags are updated when the description is.

I can set the values and check them in-memory within the model... but they're not updating in the persistence layer. Will have to have more of a look at this, maybe tomorrow.

@thombruce
Copy link
Owner Author

Actually, this is why:

db.todos.update(id, { description: todo.description, priority: todo.priority }).then().catch()

That's the saving to the database in the update function in the todos.ts Pinia store module. We're only changing description and priority.

We should prefer, I think, not to manually specify each attribute but instead pass along all attributes that are potentially changed. But I worry that there was a reason it was setup this way - thinking back, I remember having some trouble that led to this decision in the first place.

@thombruce
Copy link
Owner Author

Issue appears to be that db.todos.update will not accept arrays among the objects values. I've tested this with and without tags and with and without other values, so it's a general arrays problem not simply pertaining to the tags object of arrays and not simply pertaining to indexes existing.

Attempted to replace .update() with .put() yields same error.

Consider then that these arrays should be generated in the same way as tokens, at the Database Plugin level?

Does that feel like the right place for them? They are intended for indexing, after all. It is essentially their sole purpose...

Yeah, that's fine. It just feels like this really shouldn't be a problem...

@thombruce
Copy link
Owner Author

Yup, that works.

My one concern about that is... what about interactive tags? These are by nature intended to be individually updatable. Is that possible now that tags are being parsed at the plugin level? Something to keep in mind.

@thombruce
Copy link
Owner Author

Failing tests pertain to simple presence check of projects, contexts and tags on todos at the model layer which now has no notion of them until the Todo has been saved.

(I mean it has a notion that their keys exist, just not that the Todo has any associated tags since these will be parsed once saved.)

Something to look at tomorrow.

@thombruce
Copy link
Owner Author

Skipping those tests. They're redundant in that the model now no longer does the separation, the database does on save and update.

I would prefer if the Todo model maintained control of this, but Dexie complains a lot if I try to update array values.

We could work around this by saving objects like { 0: 'firstProject', 1: 'secondProject' }, which is an array-like object.

We could instead do something like... { 'firstProject': { ...meta }, 'secondProject': { ...meta } }, where the keys are the tag names. This is a problem if the same key is given twice on the same todo (a simple user error but a likely one), and tags might be more complicated still... since these could have multiple same keys with different values:

{ 'firstTag': { value: 'firstValue' }, 'firstTag': { value: 'secondValue' } }

Array-like object approach obviously works here, but it won't be indexable...

In fact, I don't believe either approach is indexable as this second approach depends on dynamic keys.

@thombruce
Copy link
Owner Author

I believe this is the error I was getting: https://dexie.org/docs/DexieErrors/Dexie.DataCloneError

Arrays should be supported...

@thombruce
Copy link
Owner Author

Issue is this...

Class returns Proxy Array instead of array object itself... Not sure why. Obviously it's a pain in the ass.

We can work around this at the point of the .update() call to the Dexie table by wrapping our data in JSON.parse(JSON.stringify(data)).

In fact, this means I can do this:

db.todos.update(id, JSON.parse(JSON.stringify(todo))).then().catch()

I no longer need to specify individual keys, as I was previously complaining about. That's an improvement.

@thombruce
Copy link
Owner Author

Let's also index tagKeys. We'll do this at the database level since it is purely an indexing concern.

We could also index tagValues:

`
  ...,
  *tagKeys,
  *tagValues
`

Doing so would enable us to sort tagValues alphabetically quickly using the index. Useful.

As already discussed, tags may have hidden values that are intended to be more comparable/sortable than their presented values. For instance 23m is greater than 1h23m alphabetically but would be lower as a raw time value.

Alternative to hidden values, we could enforce a formatting rule: 00h23m is less than 01h23m alphabetically. This example format would limit timer values to double-digit hours, but given that this goes up to 99 hours it's only really a problem in edge cases where a task takes more than 100 hours... certainly possible.

@thombruce
Copy link
Owner Author

How sortable are tagValues (being contained in an array) anyway...

We wouldn't be able to tell which value belongs to which key. We'd want to sort individual values from the tagValues array against other individual values from arrays on different records. And we'd need these to be identifiable by key, despite being stored here without them...

What this really calls for is a Tag model, stored separately, having :key and :value attributes. Then we could select for key and sort by value easily, then find the associated Todo(s) from an :todoID reference.

Technically the keys within the index are universally sortable (despite being stored in arrays on individual records)... but we still have the problem of telling one value belonging to a certain key apart from another belonging to a different key. Even if we filter todos by tagKey, there may still be false positives pulled into the sorting of tagValue.

Compound index [tagKeys+tagValues] could resolve this... but we'd still wind up with other tagKeys mixed into that index, causing possible interference with the sorting algorithm particularly if we can't isolate the part sorting by said tagKey...

...there's also no indication that multi-value indexes are permitted within compound keys.

I'm convinced: This is better handled by a separate Tag class, given which... it becomes redundant to index these on Todo at all.

Advise: We skip tagKeys and tagValues indexes at this time, and consider introducing a Tag class to handle these in the future.

@thombruce
Copy link
Owner Author

All of that said...

This is done! It may be entirely changed in the future - if a separate class for Tag works, it may be repurposed or lay the groundwork for Project, Context, Hashtag and perhaps Priority classes in the future. Though we should do this only if there is a reason they are necessary - as is, they should be perfectly searchable/sortable as is. Tags are unique in being comprised of more than one part.

@thombruce thombruce marked this pull request as ready for review July 2, 2023 17:12
@thombruce thombruce merged commit bdb836e into main Jul 2, 2023
@thombruce thombruce deleted the feat/tag-indexes branch July 2, 2023 17:12
@thombruce thombruce mentioned this pull request Jul 2, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature]: Index tags
1 participant