946279 - Implement autotagging of feed <category> tags

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Comment 1

[alta88]

Attachment: tag.patch

This patch attempts the following:
1) Uses the Tb tag service to tag atom/rss2 <category> values to a feed message.
2) The option is not initially enabled; in Subscribe the account folder contains global prefs as well as enables individual feeds to override the global setting.
3) Feeds may also choose to use the global setting instead of their own; this mechanism is for somewhat sane management of the 'hundreds of feeds' case.
4) Due to the potentially large quantity of <category> items that may be published, it's important a user can manage tags thus options are added to lowercase tags and add a user defined prefix and/or suffix.
5) Options are exported/saved in feed opml.

Blake, please have a ui look first..

Comment 2

[Jim Porter (:squib)]

Comment on attachment 8342453 [details] [diff] [review]
tag.patch

This appears to be the patch for bug 936524...

Comment 3

[alta88]

Attachment: category.patch

opps!

Comment 4

[alta88]

Attachment: category.patch

unbitrot and... ping.

Comment 5

[alta88]

Attachment: category.patch

Comment 6

[Blake Winton (:bwinton) (:☕️)]

I think having both a prefix and a suffix is a little overboard, and confuses the UI.  Just a prefix seems like enough variability to me.
Also, I wouldn't give people the option of lowercasing the category tags, for a similar reason.

If you feel it's absolutely necessary, perhaps it would be better to have a "Manage tags" option that let people merge tags, and lowercase them from that screen…

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8367934 [details] [diff] [review]
category.patch

Review of attachment 8367934 [details] [diff] [review]:
-----------------------------------------------------------------

(Whoops, forgot to say ui-r-.  The idea itself is solid, but I think we should start with something simpler, and only add more complicated features if people really need them…)

Thanks,
Blake.

Comment 8

[alta88]

Attachment: category.patch

I tend to think (otherwise I wouldn't have put it in) that tags created by others are very different in context than tags one creates for oneself, thus the greatest degree of user control is better.

But it's been simplified as suggested.

Comment 9

[alta88]

Attachment: categorystrings.patch

suite strings.

Comment 10

[alta88]

Attachment: category.patch

tweaks.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8393636 [details] [diff] [review]
category.patch

Review of attachment 8393636 [details] [diff] [review]:
-----------------------------------------------------------------

Can't subscribe with this patch. 

Error: TypeError: this.optionsDefault is not a function
Source File: resource:///modules/FeedUtils.jsm
Line: 515

... from the catch in getOptionsAcct

::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
@@ +18,5 @@
>  
>  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
>  <!ENTITY quickMode.accesskey         "h">
>  
> +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">

I don't see any account level autotag settings anywhere.

I think it would be better to not use "Autotag" but something more explanatory.
Something like "automatic tag creation based on category specified by feeds"

@@ +19,5 @@
>  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
>  <!ENTITY quickMode.accesskey         "h">
>  
> +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> +<!ENTITY autotagAllowOverride.label  "Allow feeds to override these account Autotag settings">

this item is always hidden ??

@@ +22,5 @@
> +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> +<!ENTITY autotagAllowOverride.label  "Allow feeds to override these account Autotag settings">
> +<!ENTITY autotagUseAccount.accesskey "a">
> +<!ENTITY autotagEnable.label         "Enable Autotag of feed &lt;category&gt; tags">
> +<!ENTITY autotagEnable.accesskey     "E">

Bbut i don't understand. It should be either account settings OR the override. Never both

@@ +25,5 @@
> +<!ENTITY autotagEnable.label         "Enable Autotag of feed &lt;category&gt; tags">
> +<!ENTITY autotagEnable.accesskey     "E">
> +<!ENTITY autotagUsePrefix.label      "Prefix tags with:">
> +<!ENTITY autotagUsePrefix.accesskey  "P">
> +<!ENTITY autoTagPrefix.label         "feeds:">

this isn't showing up...
Tb has all the other tags capitalized, so this probably should too, like "Feed-category:"

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +286,5 @@
>      // If there is an inreplyto value, create the headers.
>      let inreplytoHdrsStr = this.inReplyTo ?
>        ("References: " + this.inReplyTo + "\n" +
>         "In-Reply-To: " + this.inReplyTo + "\n") : "";
> + 

trailing whitespace

@@ +306,5 @@
> +        if (keywordsStr.length + keyword.length > MAXLEN)
> +        {
> +//          keywordsStr = keywordsStr.replace(/,$/,"");
> +          lines.push(keywordsStr)
> +//          keywordsStr = HEADER;

remove the commented out stuff

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +506,5 @@
> +  },
> +
> +  getOptionsAcct: function(aServer)
> +  {
> +    let optionsAcctPref = "mail.server." + aServer.key + ".Options";

lowercase options
but maybe it's better named feed_options

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1142,5 @@
> +        // empty folders prior to add.
> +        quickMode.disabled = false;
> +
> +        let categoryPrefsAcct = FeedUtils.getOptionsAcct(item.folder.server).category;
> +  

trailing whitespace

@@ +1154,5 @@
> +        // Enabled for a folder with feeds.
> +        quickMode.disabled = !FeedUtils.getFeedUrlsInFolder(item.folder);
> +
> +        autotagUseAccount.disabled = autotagEnable.disabled =
> +        autotagUsePrefix.disabled = autotagPrefix.disabled = true;

these two lines look odd like that. either indent the second or add true to the first line

Comment 12

[alta88]

Attachment: category.patch

Comment 13

[alta88]

(In reply to Magnus Melin from comment #11)
> Comment on attachment 8393636 [details] [diff] [review]
> category.patch
> 
> Review of attachment 8393636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't subscribe with this patch. 
> 
> Error: TypeError: this.optionsDefault is not a function
> Source File: resource:///modules/FeedUtils.jsm
> Line: 515
> 
> ... from the catch in getOptionsAcct

ack, somehow the patch didn't get updated there when it changed to a getter..

> 
> ::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
> @@ +18,5 @@
> >  
> >  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
> >  <!ENTITY quickMode.accesskey         "h">
> >  
> > +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> 
> I don't see any account level autotag settings anywhere.
> 
> I think it would be better to not use "Autotag" but something more
> explanatory.
> Something like "automatic tag creation based on category specified by feeds"
> 
> @@ +19,5 @@
> >  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
> >  <!ENTITY quickMode.accesskey         "h">
> >  
> > +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> > +<!ENTITY autotagAllowOverride.label  "Allow feeds to override these account Autotag settings">
> 
> this item is always hidden ??
> 
> @@ +22,5 @@
> > +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> > +<!ENTITY autotagAllowOverride.label  "Allow feeds to override these account Autotag settings">
> > +<!ENTITY autotagUseAccount.accesskey "a">
> > +<!ENTITY autotagEnable.label         "Enable Autotag of feed &lt;category&gt; tags">
> > +<!ENTITY autotagEnable.accesskey     "E">
> 
> Bbut i don't understand. It should be either account settings OR the
> override. Never both
> 
> @@ +25,5 @@
> > +<!ENTITY autotagEnable.label         "Enable Autotag of feed &lt;category&gt; tags">
> > +<!ENTITY autotagEnable.accesskey     "E">
> > +<!ENTITY autotagUsePrefix.label      "Prefix tags with:">
> > +<!ENTITY autotagUsePrefix.accesskey  "P">
> > +<!ENTITY autoTagPrefix.label         "feeds:">
> 
> this isn't showing up...
> Tb has all the other tags capitalized, so this probably should too, like
> "Feed-category:"
> 

much of the above should be revisited as it wouldn't work right due to the TypeError above.  in brief:

1) the account setting means the setting in the Subscribe dialog when an account level (root) folder is selected.
2) the right options are enabled/disabled based on whether an account tree item or a feed subscription tree item is selected, and the right labels are also displayed based on that.
3) i didn't add the option to the Account dialog.  should it be there?  i sort of felt it was duplication and that it made more sense to have it in a place where subscriptions were also readily updateable.
4) perhaps Blake can help with the phrasing of the labels for en.  they're changed as suggested but it seems stilted, whereas i think most people would get what Autotag of a feed category is.  up for discussion.

> ::: mailnews/extensions/newsblog/content/FeedItem.js
> @@ +286,5 @@
> >      // If there is an inreplyto value, create the headers.
> >      let inreplytoHdrsStr = this.inReplyTo ?
> >        ("References: " + this.inReplyTo + "\n" +
> >         "In-Reply-To: " + this.inReplyTo + "\n") : "";
> > + 
> 
> trailing whitespace
> 
> @@ +306,5 @@
> > +        if (keywordsStr.length + keyword.length > MAXLEN)
> > +        {
> > +//          keywordsStr = keywordsStr.replace(/,$/,"");
> > +          lines.push(keywordsStr)
> > +//          keywordsStr = HEADER;
> 
> remove the commented out stuff
> 
> ::: mailnews/extensions/newsblog/content/FeedUtils.jsm
> @@ +506,5 @@
> > +  },
> > +
> > +  getOptionsAcct: function(aServer)
> > +  {
> > +    let optionsAcctPref = "mail.server." + aServer.key + ".Options";
> 
> lowercase options
> but maybe it's better named feed_options
> 
> ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> @@ +1142,5 @@
> > +        // empty folders prior to add.
> > +        quickMode.disabled = false;
> > +
> > +        let categoryPrefsAcct = FeedUtils.getOptionsAcct(item.folder.server).category;
> > +  
> 
> trailing whitespace
> 
> @@ +1154,5 @@
> > +        // Enabled for a folder with feeds.
> > +        quickMode.disabled = !FeedUtils.getFeedUrlsInFolder(item.folder);
> > +
> > +        autotagUseAccount.disabled = autotagEnable.disabled =
> > +        autotagUsePrefix.disabled = autotagPrefix.disabled = true;
> 
> these two lines look odd like that. either indent the second or add true to
> the first line

all the above has been fixed.

Comment 14

[alta88]

Attachment: category.patch

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #13)
> (In reply to Magnus Melin from comment #11)
> > Comment on attachment 8393636 [details] [diff] [review]
> > category.patch
> > 
> > Review of attachment 8393636 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can't subscribe with this patch. 
> > 
> > Error: TypeError: this.optionsDefault is not a function
> > Source File: resource:///modules/FeedUtils.jsm
> > Line: 515
> > 
> > ... from the catch in getOptionsAcct
> 
> ack, somehow the patch didn't get updated there when it changed to a getter..
> 
> > 
> > ::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
> > @@ +18,5 @@
> > >  
> > >  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
> > >  <!ENTITY quickMode.accesskey         "h">
> > >  
> > > +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> > 
> > I don't see any account level autotag settings anywhere.
> > 
> > I think it would be better to not use "Autotag" but something more
> > explanatory.
> > Something like "automatic tag creation based on category specified by feeds"
> > 
> > @@ +19,5 @@
> > >  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
> > >  <!ENTITY quickMode.accesskey         "h">
> > >  
> > > +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> > > +<!ENTITY autotagAllowOverride.label  "Allow feeds to override these account Autotag settings">
> > 
> > this item is always hidden ??
> > 
> > @@ +22,5 @@
> > > +<!ENTITY autotagUseAccount.label     "Use account Autotag settings">
> > > +<!ENTITY autotagAllowOverride.label  "Allow feeds to override these account Autotag settings">
> > > +<!ENTITY autotagUseAccount.accesskey "a">
> > > +<!ENTITY autotagEnable.label         "Enable Autotag of feed &lt;category&gt; tags">
> > > +<!ENTITY autotagEnable.accesskey     "E">
> > 
> > Bbut i don't understand. It should be either account settings OR the
> > override. Never both
> > 
> > @@ +25,5 @@
> > > +<!ENTITY autotagEnable.label         "Enable Autotag of feed &lt;category&gt; tags">
> > > +<!ENTITY autotagEnable.accesskey     "E">
> > > +<!ENTITY autotagUsePrefix.label      "Prefix tags with:">
> > > +<!ENTITY autotagUsePrefix.accesskey  "P">
> > > +<!ENTITY autoTagPrefix.label         "feeds:">
> > 
> > this isn't showing up...
> > Tb has all the other tags capitalized, so this probably should too, like
> > "Feed-category:"
> > 
> 
> much of the above should be revisited as it wouldn't work right due to the
> TypeError above.  in brief:
> 
> 1) the account setting means the setting in the Subscribe dialog when an
> account level (root) folder is selected.

Ok, I don't think that's intuitive.

> 3) i didn't add the option to the Account dialog.  should it be there?  i

I think so, if we need a per account pref, I'm not sure we do. At least having an UI option NOT to allow feeds to override seems a bit odd. 

--- 

With the latest patch, if i enter an URL to subscribe and then choose an autotagging option the URL disappears (and it sais feed updated in the bottom of the dialog)

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8395112 [details] [diff] [review]
category.patch

Review of attachment 8395112 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
@@ +25,5 @@
> +<!ENTITY autotagEnable.label         "Enable automatic tag creation of feed &lt;category&gt; tags">
> +<!ENTITY autotagEnable.accesskey     "E">
> +<!ENTITY autotagUsePrefix.label      "Prefix tags with:">
> +<!ENTITY autotagUsePrefix.accesskey  "P">
> +<!ENTITY autoTagPrefix.label         "Feed-category:">

with a space after perhaps?

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +307,5 @@
> +        {
> +          lines.push(keywordsStr)
> +          keywordsStr = " ".repeat(HEADER.length);
> +        }
> +        keywordsStr += keyword + ",";

I notice we do it "wrong" for title too, but non-ascii should really be encoded for headers

Comment 17

[alta88]

Attachment: category.patch

Comment 18

[alta88]

(In reply to Magnus Melin from comment #15)
> > 
> > 1) the account setting means the setting in the Subscribe dialog when an
> > account level (root) folder is selected.
> 
> Ok, I don't think that's intuitive.

i always use the Subscribe dialog, rather than Account manager for feeds.

> 
> > 3) i didn't add the option to the Account dialog.  should it be there?  i
> 
> I think so, if we need a per account pref, I'm not sure we do. At least
> having an UI option NOT to allow feeds to override seems a bit odd. 
> 

in retrospect, it should be in Accounts too, for consistency.  the Account option to disallow a feed override is for the case of hundreds of feeds, and a lot have been enabled for tags, and we want a quick way to turn that off, or now start using the Accounts prefixing pref for all feeds.

> --- 
> 
> With the latest patch, if i enter an URL to subscribe and then choose an
> autotagging option the URL disappears (and it sais feed updated in the
> bottom of the dialog)

those should be disabled when the intention is to add a url at the account level, as the active buttons switch if there is focus/content in the url field.  fixed.

> > +<!ENTITY autoTagPrefix.label         "Feed-category:">
> 
> with a space after perhaps?
> 

actually, this field should have the standard placeholder behavior of textboxes, rather than a hardcoded suggestion.  updated to do so.

> ::: mailnews/extensions/newsblog/content/FeedItem.js
> @@ +307,5 @@
> > +        {
> > +          lines.push(keywordsStr)
> > +          keywordsStr = " ".repeat(HEADER.length);
> > +        }
> > +        keywordsStr += keyword + ",";
> 
> I notice we do it "wrong" for title too, but non-ascii should really be
> encoded for headers

fortunately, RFC6532[1] has fixed this problem with 5322, and we should definitely support it.

[1] https://tools.ietf.org/html/rfc6532#section-3.2

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8395467 [details] [diff] [review]
category.patch

Review of attachment 8395467 [details] [diff] [review]:
-----------------------------------------------------------------

Some observations:
 - can't choose the feed settings before subscribed too (after it's verified the options get enabled)
 - prefix box should get disabled when appropriate
 - use account settings isn't the default
 - I wonder if it would be better to add a type to tags instead of using a prefix - that could be a lot more flexible
 - the "account settings" are still (also) in the subscribe dialog
 - at least personally I don't think we need the "allow override" option

Comment 20

[alta88]

(In reply to Magnus Melin from comment #19)
> Comment on attachment 8395467 [details] [diff] [review]
> category.patch
> 
> Review of attachment 8395467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some observations:
>  - can't choose the feed settings before subscribed too (after it's verified
> the options get enabled)

you can if adding a url to a subfolder, but not to an account by design; the options are different.  adding to an account uses whatever the account setting is already set to, if you look at the new feed url item post subscribe.  intention is it can be changed quite simply after subscribe completes; the tree item is even autoselected.

>  - prefix box should get disabled when appropriate

it does in the latest patch, otherwise STR?

>  - use account settings isn't the default

wfm if feed overrides are not allowed.  if they are, then the feed's tagging is enabled if the account is enabled (the user has already chosen what it should be for a new feeds) and Use Acount isn't checked by design.

>  - I wonder if it would be better to add a type to tags instead of using a
> prefix - that could be a lot more flexible

not sure what this means..

>  - the "account settings" are still (also) in the subscribe dialog

i think they should be; Summary is there already.  are you saying a user should have to open Account to manage root folder options?  that is very inefficient.  plus it's a modal dialog, it would make managing these options with 2 dialogs unworkable.

>  - at least personally I don't think we need the "allow override" option

the Account prefs cannot apply to all feeds, either tagging enabled on/off or user selected prefix (likely to be a mnemonic to the feed's name).  feeds are enormously variable in things like this and one size won't fit all.

Comment 21

[alta88]

Attachment: category.patch

Comment 22

[alta88]

This patch reworks the override design.  In retrospect, the attempt to somewhat handle mass changes wasn't actually the best way; the best/most precise solution is multiselect in the tree (bug 308434).

So the account value will be used only for UI less subscriptions, ie dnd to folderpane or commandline, just like the quickmode property is done currently.  New feeds in subscribe dialog will get whatever the user chooses in the UI at sub time.

Comment 23

[alta88]

Attachment: category.patch

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8404399 [details] [diff] [review]
category.patch

Review of attachment 8404399 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, things still to do:
 - feed-subscriptions.dtd for suite/
 - the prefix value isn't properly disabled when unticking the checkbox for use prefix

Besides that i'm fairly happy. 
Instead of using the feed prefix, I still think a more elegant way would be to let the tags have some flag so you could easily filter them out when you want. That's a lot more work of course.

::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
@@ +18,5 @@
>  
>  <!ENTITY quickMode.label             "Show the article summary instead of loading the web page">
>  <!ENTITY quickMode.accesskey         "h">
>  
> +<!ENTITY autotagEnable.label         "Enable automatic tag creation of feed &lt;category&gt; tags">

Could we make this something like
"Automatically create tags from feed category names"

(Enable sounds so techy)

@@ +22,5 @@
> +<!ENTITY autotagEnable.label         "Enable automatic tag creation of feed &lt;category&gt; tags">
> +<!ENTITY autotagEnable.accesskey     "E">
> +<!ENTITY autotagUsePrefix.label      "Prefix tags with:">
> +<!ENTITY autotagUsePrefix.accesskey  "P">
> +<!ENTITY autoTagPrefix.placeholder   "Enter a custom value">

custom value => tag prefix

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +614,5 @@
>  
> +/**
> + * This object will contain all feed specific properties.
> + *
> + */

nit: empty line

::: mailnews/extensions/newsblog/content/am-newsblog.js
@@ +22,5 @@
>    title.setAttribute("title", titleValue);
>    document.title = titleValue;
>  
>    onCheckItem("server.biffMinutes", ["server.doBiff"]);
> +  

nit: whitespace

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1054,5 @@
>      item.quickMode = aChecked;
>      let message = FeedUtils.strings.GetStringFromName("subscribe-feedUpdated");
>      this.updateStatusItem("statusText", message);
>    },
> + 

nit: whitespace

@@ +1062,5 @@
> +    if (!item)
> +      return;
> +
> +    let id = aNode.id;
> +    let checked = aNode.checked;

I'd prefer not to have these as intermediate varables here. Just use aNode.id when you need id (the one time). Makes it easer to read.

@@ +1064,5 @@
> +
> +    let id = aNode.id;
> +    let checked = aNode.checked;
> +    let isServer = item.folder && item.folder.isServer;
> +    let isFolder = item.folder && !item.folder.isServer;

isFolder = !isServer no? so you don't need that too. And item.folder is always true for feeds right?

@@ +1072,5 @@
> +    if (isFolder || (isServer && document.getElementById("locationValue").value))
> +    {
> +      // Intend to subscribe a feed to a folder, a value must be in the url
> +      // field. Update states for addFeed() and return.
> +      autotagUsePrefix.disabled = autotagPrefix.disabled = !autotagEnable.checked;

Xan you break it up to two lines. Personally i find this style very hard to read.

@@ +1364,3 @@
>      return feed;
>    },
> + 

nit: whitespace

@@ +1383,5 @@
>      if (seln.count != 1)
>        return;
>  
>      let itemToEdit = this.mView.getItemAtIndex(seln.currentIndex);
> +    if (itemToEdit.folder && itemToEdit.folder.isServer)

when do we not have a folder?

@@ +1438,5 @@
>        feed.quickMode = editQuickMode;
>        itemToEdit.quickMode = editQuickMode;
>        updated = true;
>      }
> + 

nit: whitespace

Comment 25

[alta88]

(In reply to Magnus Melin from comment #24)
> Comment on attachment 8404399 [details] [diff] [review]
> category.patch
> 
> Review of attachment 8404399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Alright, things still to do:
>  - feed-subscriptions.dtd for suite/
>  - the prefix value isn't properly disabled when unticking the checkbox for
> use prefix
> 
> Besides that i'm fairly happy. 
> Instead of using the feed prefix, I still think a more elegant way would be
> to let the tags have some flag so you could easily filter them out when you
> want. That's a lot more work of course.

i think what you mean (flag to me is a bit/byte) is to have additional tagging of custom user values, in addition to the tags found in the feed.

this was considered and not done since the current way to tag custom values to messages is using filters so there would be non consistency of tagging for different message types. 

but, in retrospect, custom tag(s) here (comma separated list) is perhaps preferable to prefix.  it's not hard at all; pls advise if this is the way to go.

Comment 26

[alta88]

also, if the prefix is turned into custom tags, it should be its own option not dependent on whether feed tags are enabled, no?

Comment 27

[Magnus Melin [:mkmelin]]

Not sure I understood what you mean but if it would be a "system-tag" prefixing probably has little value. I don't think we'd necessarily need an option to use system-tag or not. But sure, I'd go with system tags.

Comment 28

[alta88]

hmm.  let's say we have a moz planet feed.  some items have <category> Tech and Design.  with prefixing, a user could add a prefix "Planet:" to get Planet:Tech and Planet:Design tags in the message.

if prefixing is changed in favor of custom tags, a user could add "Planet,Mozilla" in the feed and end up with tags Planet, Mozilla, Tech, Design in the message.

which is better?

(otherwise i don't know what you mean by system tags).

Comment 29

[alta88]

althought the second is possible now, using the feed url and a custom filter Content-Base to add custom tags..

Comment 30

[Magnus Melin [:mkmelin]]

What i meant with "system tags" was that these auto-created tags would have a property identifying them as say auto-created. The UI could then choose not to show those for manual tagging or what not, but could still use them for filtering/display as normal.

Comment 31

[alta88]

ah.  that's different from a user being able to impart extra info to tags via a prefix etc.

the tag api is something else, each current tag 'property' is an additional pref.  i guess (json) objects were unknown back then.  changing this api for an additional property would properly mean a rewrite of how they are stored...

there may be value in using the ordinal property, an ACString, meant for sort order and only used by an extension, to store something like ~AUTOTAG.  a UI like menulists could key off this.

what do you think?  i think its a hack but a righteous hack to categorize/identify autotags from the start.

Comment 32

[Magnus Melin [:mkmelin]]

Sounds ok to me!

Comment 33

[alta88]

(In reply to Magnus Melin from comment #24)
> Comment on attachment 8404399 [details] [diff] [review]
> category.patch
> 
> Review of attachment 8404399 [details] [diff] [review]:
> 
> isFolder = !isServer no? so you don't need that too. And item.folder is
> always true for feeds right?

no, for a feed row object the property is 'parentFolder'.

all other items fixed.

Comment 34

[alta88]

Attachment: category.patch

Comment 35

[alta88]

Attachment: categorystrings.patch

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8405864 [details] [diff] [review]
category.patch

Okay, I think this is good enough to run with, from a ui point of view.
ui-r=me.

Comment 37

[Philip Chee]

Comment on attachment 8405865 [details] [diff] [review]
categorystrings.patch

Looks reasonable. r=me

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 8405864 [details] [diff] [review]
category.patch

Review of attachment 8405864 [details] [diff] [review]:
-----------------------------------------------------------------

The prefix value field under account settings still doesn't respect disabling. r=mkmelin with that fixed

So, what about the ~autotag thing?

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +418,5 @@
> +      }
> +      catch(ex) {
> +        FeedUtils.log.trace("FeedItem.tagItem: failed to tag - <" + keyword +
> +                            "> to Subject: " + this.title);
> +      }

doesn't look like this try/catch would ever (under normal conditions) get exercised, so better remove it

Comment 39

[alta88]

(In reply to Magnus Melin from comment #38)
> Comment on attachment 8405864 [details] [diff] [review]
> category.patch
> 
> Review of attachment 8405864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The prefix value field under account settings still doesn't respect
> disabling. r=mkmelin with that fixed

fixed.

> 
> So, what about the ~autotag thing?
> 

it's implemented with:
+  // Add the tag if it doesn't exist.
+  MailServices.tags.addTag(keyword, "", FeedUtils.AUTOTAG);

a followup would adjust lists/tag mgmt dialog to make use of it.

> ::: mailnews/extensions/newsblog/content/FeedItem.js
> @@ +418,5 @@
> > +      }
> > +      catch(ex) {
> > +        FeedUtils.log.trace("FeedItem.tagItem: failed to tag - <" + keyword +
> > +                            "> to Subject: " + this.title);
> > +      }
> 
> doesn't look like this try/catch would ever (under normal conditions) get
> exercised, so better remove it

ok.

Comment 40

[alta88]

Attachment: category.patch

updated.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/937875025e76
https://hg.mozilla.org/comm-central/rev/e52da7ad68d1