feat: more categorization and support for select_keys #286

iloveitaly · 2021-05-05T19:18:33Z

Similar to this PR (and as discussed here this contains a additional app categorizations.

Some additional changes:

Added select_keys to the rule. This is helpful for matching on the domain name of an event.
Added prettier colors (screenshot below)

codecov · 2021-05-05T19:21:11Z

Codecov Report

Base: 25.05% // Head: 25.40% // Increases project coverage by +0.35% 🎉

Coverage data is based on head (5073642) compared to base (12effc6).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   25.05%   25.40%   +0.35%     
==========================================
  Files          27       27              
  Lines        1489     1496       +7     
  Branches      240      247       +7     
==========================================
+ Hits          373      380       +7     
  Misses       1086     1086              
  Partials       30       30

Impacted Files	Coverage Δ
src/util/classes.ts	`82.40% <100.00%> (+1.21%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

iloveitaly · 2022-02-04T17:27:55Z

@ErikBjare friendly reminder on this PR :)

Before I fix the last failing test, I want to make sure this is the sort of thing you'd be ok merging in.

ErikBjare · 2022-02-08T12:00:57Z

src/util/classes.ts

+  {
+    name: ['Work', 'Programming'],
+    rule: { type: 'regex', select_keys: ['url'], regex: 'github.com' },
+    data: { color: COLOR_SUPER_GREEN },
+  },


Does having several category rules with the same name work correctly in the web UI? (editing etc)

Seems redundant to specify data several times (how would collisions/differences be handled?).

It does seem to work from my testing!

This rule will only work in those cases where there is a url set on events at all (only on macOS), which makes me not want this as a default rule (esp with the "hidden" select_keys specifier).

I think it will be very confusing to users which would expect this to work, given it's a default, but for most users it won't (and without a select_keys UI, it would be a mystery why it doesn't work if they were to e.g. edit it).

ErikBjare · 2022-02-08T12:07:46Z

src/util/classes.ts

-    data: { color: '#F80' },
+    data: { color: COLOR_RED },


Was orange, now red (intentional/improvement?)

Suggested change

data: { color: '#F80' },

data: { color: COLOR_RED },

data: { color: COLOR_ORANGE },

ErikBjare · 2022-02-08T12:09:21Z

src/util/classes.ts

-    data: { color: '#F33' },
+    data: { color: COLOR_RED },


Was a dimmer/desaturated red, now a darker/saturated red. Worse contrast with the black text.

ErikBjare · 2022-02-08T12:09:48Z

src/util/classes.ts

-    data: { color: '#FCC400' },
+    data: { color: COLOR_RED },


Was yellow, now red.

I can revert it back, but I think the dark red looks ok:

Maybe just personal preference on my end, so happy to revert if you'd like.

ErikBjare · 2022-02-08T12:10:07Z

src/util/classes.ts

  },
  {
    name: ['Media', 'Music'],
    rule: {
      type: 'regex',
-      regex: 'Spotify|Deezer',
+      regex: 'Spotify|Deezer|Amazon Music',
      ignore_case: true,
    },
    data: { color: '#A8FC00' },


Can this color be lifted out into a constant too?

ErikBjare · 2022-02-08T12:11:33Z

src/util/classes.ts

+const COLOR_GREEN = '#96cd39',
+  COLOR_SUPER_GREEN = '#54e346',
+  COLOR_UNCAT = '#CCC',
+  COLOR_ORANGE_GREEN = '#f5ff65',


Maybe rename/change this slightly to a yellow?

iloveitaly · 2022-06-14T14:36:15Z

@ErikBjare Finally getting back to this, take another look and let me know what you think!

ErikBjare

Just a couple minor things and I'll merge :)

ErikBjare · 2022-07-06T14:10:22Z

src/util/classes.ts

+    name: ['Work', 'Writing'],
+    rule: {
+      type: 'regex',
+      select_keys: ['app'],


So this is set, but there is no way to change it in the UI?

All select_keys should probably be removed from the default categories until the UI supports it.

Yes, that's right.

You can't set them via the UI, but you can import a config file which contains them. I think we should add select_keys to the default config since you can 'export, modify, import' and to mutate these keys and it makes it more explicit how we are deciding which app is in use.

But that's very confusing for the user. They have no indication there is even a select_keys for those categories, right? (unless they export and inspect)

I really don't want to add this specifier to the default rules without:

UI support for it

More thorough testing of these new "multi-rule" categories in the UI.

What happens in the UI if a category with multiple rules has children?

What happens when you rename a category with multiple rules? Or assign it to a different parent?

What happens to color keys when you have multiple rules?

Ideally, a category with multiple rules should be listed only once in the category tree, with an indication that there are multiple rules for this one category (without listing it as two seperate categories, since that's likely to get confusing).

ErikBjare · 2022-07-06T14:10:44Z

src/util/classes.ts

+// https://colorhunt.co/palette/4802
+const COLOR_GREEN = '#96cd39',
+  COLOR_SUPER_GREEN = '#54e346',
+  COLOR_BRIGHT_GREEN = '#A8FC00',
+  COLOR_UNCAT = '#CCC',
+  COLOR_SLIGHTLY_YELLOW = '#f5ff65',
+  COLOR_ORANGE = '#ffba47',
+  COLOR_RED = '#ff5b44';


Please revert these to previous colors (but good to keep them as constants!)

src/util/classes.ts

iloveitaly · 2022-12-19T16:28:05Z

@ErikBjare finally fixed this. Collapsed the commits & rebased. Hopefully this is what we need to get this in!

ErikBjare

Was hoping to merge this but I still find the hidden-from-user select_keys behavior problematic.

I really want to see a UI for it before I feel okay with adding the selector to the default categories, as I otherwise think users will be very confused.

The "multi-rule" categories also make me uneasy, as I didn't design for it so I would want to review/test this much more thoroughly before I feel confident merging it (see comments for specifics).

To support multi-rule categories like this without causing trouble, I think it'd be best if we allowed the rule property to be a array of rules. Instead of having several categories with the same name implicitly be the same (troublesome with the data property, renaming, parents, etc).

All these color changes are also very hard to review. I generally don't want them changed. I tried to leave comments where I found differences, but I'm sure I didn't spot them all.

If you want, we can strip the select_keys and merge this now (so we get the color constants etc.), or we can leave this open if you want to give the UI part a shot.

ErikBjare · 2022-12-23T08:59:08Z

src/util/classes.ts

+  {
+    name: ['Work', 'Programming'],
+    rule: { type: 'regex', select_keys: ['url'], regex: 'github.com' },
+    data: { color: COLOR_SUPER_GREEN },
+  },


This rule will only work in those cases where there is a url set on events at all (only on macOS), which makes me not want this as a default rule (esp with the "hidden" select_keys specifier).

I think it will be very confusing to users which would expect this to work, given it's a default, but for most users it won't (and without a select_keys UI, it would be a mystery why it doesn't work if they were to e.g. edit it).

ErikBjare · 2022-12-23T08:59:55Z

src/util/classes.ts

+  },
+  {
+    name: ['Work', 'General'],
+    rule: {
+      type: 'regex',
+      regex: 'Preview|Finder|Todoist|1Password|Soulver|System Preferences|VNC Viewer|Streaks',
+      select_keys: ['app'],
+    },
+    data: { color: COLOR_SLIGHTLY_YELLOW },


Suggested change

},

{

name: ['Work', 'General'],

rule: {

type: 'regex',

regex: 'Preview|Finder|Todoist|1Password|Soulver|System Preferences|VNC Viewer|Streaks',

select_keys: ['app'],

},

data: { color: COLOR_SLIGHTLY_YELLOW },

Sorry, but I don't like this as a default category. It's too broad to be generally useful, imo. Better to leave underspecified stuff like this up to the user (or if one would keep it, it would belong better in the parent category 'Work', such that child-categories can override).

ErikBjare · 2022-12-23T09:07:13Z

src/util/classes.ts

+      regex:
+        'Messenger|Messages|Discord|Telegram|Signal|WhatsApp|Rambox|Slack|Riot|Element|Discord|Textual|Nheko|Texts',
+    },
+    data: { color: COLOR_ORANGE },


Why override the #9FF comms color with something very different?

Suggested change

data: { color: COLOR_ORANGE },

ErikBjare · 2022-12-23T09:08:31Z

src/util/classes.ts

+  {
+    name: ['Comms', 'Email'],
+    rule: { type: 'regex', regex: 'Gmail|Thunderbird|mutt|alpine' },
+    data: { color: COLOR_ORANGE },


Same as above.

Suggested change

data: { color: COLOR_ORANGE },

ErikBjare · 2022-12-23T09:08:39Z

src/util/classes.ts

+  {
+    name: ['Comms', 'Meetings'],
+    rule: { type: 'regex', regex: 'Zoom|Calendar|Cron' },
+    data: { color: COLOR_SLIGHTLY_YELLOW },


Suggested change

data: { color: COLOR_SLIGHTLY_YELLOW },

ErikBjare · 2022-12-23T09:10:14Z

src/util/classes.ts

@@ -197,6 +268,7 @@ function pickDeepest(categories: Category[]) {
  return _.maxBy(categories, c => c.name.length);
 }

+// TODO this is only used colorize the categories, all categorization is done on the backend


Suggested change

// TODO this is only used colorize the categories, all categorization is done on the backend

// NOTE: this is only used to colorize the categories, all actual categorization is done on the backend

ErikBjare · 2022-12-23T09:12:14Z

src/util/classes.ts

+    name: ['Work', 'Writing'],
+    rule: {
+      type: 'regex',
+      select_keys: ['app'],


But that's very confusing for the user. They have no indication there is even a select_keys for those categories, right? (unless they export and inspect)

I really don't want to add this specifier to the default rules without:

UI support for it

More thorough testing of these new "multi-rule" categories in the UI.

What happens in the UI if a category with multiple rules has children?

What happens when you rename a category with multiple rules? Or assign it to a different parent?

What happens to color keys when you have multiple rules?

Ideally, a category with multiple rules should be listed only once in the category tree, with an indication that there are multiple rules for this one category (without listing it as two seperate categories, since that's likely to get confusing).

ErikBjare · 2022-12-23T09:25:40Z

src/util/classes.ts

+  COLOR_YELLOW = '#FCC400',
+  COLOR_SLIGHTLY_YELLOW = '#f5ff65',
+  COLOR_ORANGE = '#ffba47',
+  COLOR_RED = '#F80';


Red is orange now? Previous red was #F33.

Suggested change

COLOR_RED = '#F80';

COLOR_RED = '#F33';

ErikBjare · 2022-12-23T09:26:48Z

src/util/classes.ts

-    name: ['Work'],
-    rule: { type: 'regex', regex: 'Google Docs|libreoffice|ReText' },
-    data: { color: '#0F0' },


Better to keep this parent-category explicit imo. Even if without a rule, such that children inherit color and don't need to be set explicitly for each child.

ErikBjare · 2022-12-23T09:30:20Z

src/util/classes.ts

-    data: { color: '#F80' },
+    data: { color: COLOR_RED },


Suggested change

data: { color: '#F80' },

data: { color: COLOR_RED },

data: { color: COLOR_ORANGE },

iloveitaly · 2022-12-24T13:52:54Z

Makes sense! I don't have any additional time to spend on this, so someone else will have to take this up to get it across the finish line.

CVirus · 2023-05-31T09:33:04Z

bump

iloveitaly · 2023-10-22T12:52:22Z

Closing out in favor of the simplier #495

iloveitaly force-pushed the more-categorization branch from 19378fe to ec49a1d Compare January 19, 2022 14:32

iloveitaly changed the title ~~More categorization and support for select_keys~~ feat: more categorization and support for select_keys Jan 19, 2022

ErikBjare reviewed Feb 8, 2022

View reviewed changes

iloveitaly force-pushed the more-categorization branch from 9a4ec7b to 9fe3fe0 Compare June 14, 2022 14:30

ErikBjare requested changes Jul 6, 2022

View reviewed changes

ErikBjare mentioned this pull request Jul 13, 2022

Remaining tasks after categorization PR #151

Open

7 tasks

soxofaan reviewed Aug 10, 2022

View reviewed changes

src/util/classes.ts Outdated Show resolved Hide resolved

ErikBjare force-pushed the master branch from 871a9e1 to 39de143 Compare August 12, 2022 13:32

iloveitaly added 2 commits December 19, 2022 09:24

More categorization and support for select_keys

15bdfa2

revert colors

5073642

iloveitaly force-pushed the more-categorization branch from fa77b08 to 5073642 Compare December 19, 2022 16:27

ErikBjare requested changes Dec 23, 2022

View reviewed changes

ErikBjare force-pushed the master branch from 039c639 to ce5edc4 Compare March 22, 2023 18:43

iloveitaly closed this Oct 22, 2023

	data: { color: '#F80' },
	data: { color: COLOR_RED },
	data: { color: COLOR_ORANGE },

	// TODO this is only used colorize the categories, all categorization is done on the backend
	// NOTE: this is only used to colorize the categories, all actual categorization is done on the backend

feat: more categorization and support for select_keys #286

feat: more categorization and support for select_keys #286

Conversation

iloveitaly commented May 5, 2021

codecov bot commented May 5, 2021 • edited Loading

Codecov Report

iloveitaly commented Feb 4, 2022

ErikBjare Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveitaly commented Jun 14, 2022

ErikBjare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveitaly commented Dec 19, 2022

ErikBjare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikBjare Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveitaly commented Dec 24, 2022

CVirus commented May 31, 2023

iloveitaly commented Oct 22, 2023

codecov bot commented May 5, 2021 •

edited

Loading

ErikBjare Feb 8, 2022 •

edited

Loading

ErikBjare Feb 8, 2022 •

edited

Loading

ErikBjare Feb 8, 2022 •

edited

Loading

ErikBjare Dec 23, 2022 •

edited

Loading

ErikBjare Dec 23, 2022 •

edited

Loading