Bugzilla

Assignee

Comment 13

•

15 years ago

Attached patch Make newsgroups in header clickable (obsolete) — Details — Splinter Review

Attached is a patch that linkifies newsgroups in message headers. Unlike the email header field, this doesn't bother with caching the results (it might be worth doing this). It also reuses the "Compose Mail To" string from the email link's popup; maybe it should have a new string of its own...

Also, I hope I requested a review from an appropriate person. If not, feel free to change it. :)

Attachment #395679 - Flags: review?(clarkbw)

Comment 14

•

15 years ago

Comment on attachment 395679 [details] [diff] [review]
Make newsgroups in header clickable

wow, that is awesome.  I'm not the right person to ask about this but I can make sure the right people get cc'd :)

Attachment #395679 - Flags: review?(clarkbw)

Comment 15

•

15 years ago

can one of you guys take this on for review?

David :Bienvenu

Updated

•

15 years ago

Attachment #395679 - Flags: review?(dmose)

Assignee

Comment 16

•

15 years ago

Attached patch Add "Subscribe to Newsgroup" to popup menu (obsolete) — Details — Splinter Review

I added the option to subscribe to a newsgroup if you aren't currently subscribed to it (really only useful for cross-posted stuff or followup-to). If you're currently subscribed to the newsgroup, it disables that menu item, since 1) switching it to "unsubscribe" seems dangerous, and 2) people would probably get annoyed if the menu items changed position based on the subscription-state of a newsgroup.

Speaking of followup-to, is that field supposed to be used for email addresses *and* newsgroups, or just one of those (and if so, which?). The followup-to field was using OutputNewsgroups to display its contents, but on reading about it more, it seems like it's not meant solely for newsgroups. I still think followup-to should be more than just a static list, but if it's as I'm guessing, fixing that is probably a separate bug (e.g. bug 162294).

Attachment #395679 - Attachment is obsolete: true

Attachment #395679 - Flags: review?(dmose)

Blake Winton (:bwinton) (:☕️)

Assignee

Updated

•

15 years ago

Attachment #395895 - Flags: review?(dmose)

Magnus Melin [:mkmelin]

Updated

•

15 years ago

Assignee: nobody → squibblyflabbetydoo

Status: NEW → ASSIGNED

Target Milestone: Future → Thunderbird 3.0b4

Comment 17

•

15 years ago

What about always having Subscribe and Unsubscribe, and only enabling the one that's valid?  That way they won't change position, but you could unsubscribe if you wanted to…

(I think I prefer not having it there, but I wanted to throw the idea out just in case.)

Assignee

Comment 18

•

15 years ago

Yeah, I'm not sure how useful the "Subscribe" item is in practice, but I added it in to match up with the Address Book items for email addresses. I opted to disable the menu item instead of switching to "Unsubscribe" since the Address Book option for a contact already in your book is "edit" instead of "delete". Since there's nothing to edit in a newsgroup subscription, I just disabled it.

I also have a feeling that if Unsubscribe were an option, I'd accidentally click it and then be annoyed.

Finally, my patch doesn't seem to want to acknowledge the subscription, which is my own dumb fault for not doublechecking that when I changed how it subscribes!

Magnus Melin [:mkmelin]

Comment 19

•

15 years ago

I didn't try the patch, but if you go to a newsgroup you're not subscribed to, you will get asked to subscribe, no? In any case, i agree an Unsubscribe entry isn't useful at that place.

Assignee

Comment 20

•

15 years ago

(In reply to comment #19)
> I didn't try the patch, but if you go to a newsgroup you're not subscribed to,
> you will get asked to subscribe, no? In any case, i agree an Unsubscribe entry
> isn't useful at that place.

Via a URL, I think so.

If you're actually reading a newsgroup, the only time subscription would make sense (I think) is when someone cross-posted a message. In my opinion, subscription is only remotely useful in addressing bug 162294 (subscribing to a newsgroup from the Followup-To field).

Assignee

Comment 21

•

15 years ago

Attached patch Add unsubscribe option (obsolete) — Details — Splinter Review

Added unsubscription (and fixed subscribing so it actually works). I don't really like unsubscription being there, and I'm pretty neutral on subscription (except, again, to address bug 162294).

Attachment #395895 - Attachment is obsolete: true

Attachment #395895 - Flags: review?(dmose)

Comment 22

•

15 years ago

(In reply to comment #13)
> Created an attachment (id=395679) [details]
> Make newsgroups in header clickable
> 
> Attached is a patch that linkifies newsgroups in message headers. Unlike the
> email header field, this doesn't bother with caching the results (it might be
> worth doing this). 

Great; thanks for the patch!  I'd like to start off by getting the UI nailed down, and then move on to the review.  So I'll start with my comments on the UI, and ask for Bryan to either sign off on them or correct me/propose something better.

> It also reuses the "Compose Mail To" string from the email
> link's popup; maybe it should have a new string of its own...

I would propose that we change the existing string to "Compose Message To", because this works not only for mail and news, but also for messages that get transmitted by both media, as we often do in Mozilla.  Bryan?

> Speaking of followup-to, is that field supposed to be used for email addresses
> *and* newsgroups, or just one of those (and if so, which?). The followup-to
> field was using OutputNewsgroups to display its contents, but on reading about
> it more, it seems like it's not meant solely for newsgroups. I still think
> followup-to should be more than just a static list, but if it's as I'm
> guessing, fixing that is probably a separate bug (e.g. bug 162294).

Followup-To is for news only, and Reply-To is for email only.  The semantics for messages which contain both headers aren't nailed down very well.  There was an old internet-draft which was intended to address it, but it never went anywhere.

(In reply to comment #21)
> Created an attachment (id=395923) [details]
> Add unsubscribe option
> 
> Added unsubscription (and fixed subscribing so it actually works). I don't
> really like unsubscription being there, and I'm pretty neutral on subscription
> (except, again, to address bug 162294).

My suspicion is that "subscribe" from a cross-posted group in the header would be used enough that it might be worth having there, though I don't feel particularly strongly about.  

I would guess that, on balance, even if we had an unsubscribe item, the vast majority of the time people would use some other newsgroup-focused (as opposed to message-focused) UI to unsubscribe anyway.  Furthermore, they'd probably hit it in error some of the time and be annoyed at the modal dialog it would pop up.  So my suggestion would just be to not have the any subscription-related menu item for cases where the person is already subscribed.

clarkbw, can you offer an ui-r+ on this or something like it?

Assignee

Comment 23

•

15 years ago

(In reply to comment #22)
> Followup-To is for news only, and Reply-To is for email only.  The semantics
> for messages which contain both headers aren't nailed down very well.  There
> was an old internet-draft which was intended to address it, but it never went
> anywhere.

Then there's also "Mail-Followup-To", right? (And "Mail-Reply-To", for that matter). I think that's where I was getting confused, since those are for mail, whereas Followup-To and Reply-To are for news (but Reply-To is also for mail and means something different, I guess).

Nevertheless, it looks like Followup-To is meant to contain only newsgroups, which makes my life easy. :)

(In reply to comment #22)
> My suspicion is that "subscribe" from a cross-posted group in the header would
> be used enough that it might be worth having there, though I don't feel
> particularly strongly about.  
> 
> I would guess that, on balance, even if we had an unsubscribe item, the vast
> majority of the time people would use some other newsgroup-focused (as opposed
> to message-focused) UI to unsubscribe anyway.  Furthermore, they'd probably hit
> it in error some of the time and be annoyed at the modal dialog it would pop
> up.  So my suggestion would just be to not have the any subscription-related
> menu item for cases where the person is already subscribed.

Yeah, I'd probably use the subscribe item once in a while, but I could never imagine using the unsubscribe item. My only concern would be that if the the subscribe item was deleted when it was irrelevant, it could cause minor confusion when dealing with unsubscribed groups. If people were used to "Compose Message To" being first, they might get in the habit of clicking the first item to compose a message to that newsgroup, but if you do that with an unsubscribed group, you'd just end up subscribing. On the other hand, I doubt people often post to newsgroups that they aren't subscribers of, so maybe it's not really an issue.

Comment 24

•

15 years ago

(In reply to comment #22)
> (In reply to comment #13)
> > It also reuses the "Compose Mail To" string from the email
> > link's popup; maybe it should have a new string of its own...
> 
> I would propose that we change the existing string to "Compose Message To",
> because this works not only for mail and news, but also for messages that get
> transmitted by both media, as we often do in Mozilla.  Bryan?

Sounds good.

> (In reply to comment #21)
> > Created an attachment (id=395923) [details] [details]
> > Add unsubscribe option
> > 
> > Added unsubscription (and fixed subscribing so it actually works). I don't
> > really like unsubscription being there, and I'm pretty neutral on subscription
> > (except, again, to address bug 162294).
> 
> My suspicion is that "subscribe" from a cross-posted group in the header would
> be used enough that it might be worth having there, though I don't feel
> particularly strongly about.  
> 
> I would guess that, on balance, even if we had an unsubscribe item, the vast
> majority of the time people would use some other newsgroup-focused (as opposed
> to message-focused) UI to unsubscribe anyway.  Furthermore, they'd probably hit
> it in error some of the time and be annoyed at the modal dialog it would pop
> up.  So my suggestion would just be to not have the any subscription-related
> menu item for cases where the person is already subscribed.
> 
> clarkbw, can you offer an ui-r+ on this or something like it?

Agreed.  I think the subscribe option offers a lot of value for getting started in newsgroups.  It could appear at the bottom of the menulist so it was out of the way of normal use but still available for when it was wanted.  I wouldn't leave it there as disabled (when already subscribed) since I would be worried that a person would think they can't subscribe instead of just not having seeing the option to subscribe to a list they area already subscribed to.

I assume the unsubscribe action is used far less than anything else.  Newsgroups in TB are very passive (by default) requiring attention in order to update them so I can't see people needing a quick unsubscribe as nothing happens if you aren't using them.  And you make good points about a dialog we would need but could get annoying.

Assignee

Comment 25

•

15 years ago

Attached patch Move subscription item, eliminate unsub (obsolete) — Details — Splinter Review

Moved the subscription item to the bottom, got rid of the unsubscription item, and changed the "Compose Mail To" label to "Compose Message To". I also renamed the identifier for that label (SendMailTo -> SendMessageTo), since I gather that's necessary for L10N to notice the change.

Attachment #395923 - Attachment is obsolete: true

Assignee

Comment 26

•

15 years ago

Attached image Clicking an unsubscribed group — Details

Assignee

Comment 27

•

15 years ago

Attached image Clicking a subscribed group — Details

Comment 28

•

15 years ago

Comment on attachment 395992 [details] [diff] [review]
Move subscription item, eliminate unsub

looks great to me!  nice work.

Attachment #395992 - Flags: ui-review+

Updated

•

15 years ago

Attachment #395992 - Flags: review?(dmose)

Comment 30

•

15 years ago

Marking as private just because it's a half-finished comment; the real one will appear here soon!

Updated

•

15 years ago

Attachment #395992 - Flags: review?(dmose) → review-

Comment 31

•

15 years ago

Comment on attachment 395992 [details] [diff] [review]
Move subscription item, eliminate unsub

Thanks for the patch, and sorry for the review delay!

What motivated the decision to create what's effectively a fork of <mail-multi-emailHeaderField> and <mail-emailaddress>(as opposed to generalizing and/or specializing the existing bindings)?  I'm not saying that the decision to fork is the wrong one, but it feels like there's a lot of duplicated code here, so I'd like to understand it better...

For added functions/methods that aren't completely trivial, I'd like to request that you include detailed doxygen-style comments above them specifying behavior, parameters, and return values.  This significantly eases the lives of folks who want to use, read, or maintain the code.  It's particularly useful during maintenance, because it makes it possible to tell if specific behavior was intentional or a bug that should be fixed.
As you've undoubtedly noticed, most of the code in this file does quite poorly at this, but I don't want code that we add to compound what's already a bad problem.

>+  <binding id="mail-newsgroup-headerfield">

How about naming this "mail-newsgroups-headerfield"? I think that would be more clear/accurate to folks reading the code.

>+    <content>
>+      <xul:hbox class="headerNameBox" align="baseline">
>+        <xul:label class="headerName" xbl:inherits="value=label" control="headerValue" flex="1"/>
>+      </xul:hbox>
>+
>+      <xul:hbox class="headerValueBox" anonid="newsgroups" flex="1"
>+                singleline="true"
>+                align="baseline">
>+      </xul:hbox>
>+      <!--<xul:mail-newsgroup class="headerValue" anonid="newsgroupNode"/>-->

I assume the above comment is intended to be an example of simple anonymous
content that gets built up by the XBL?  If you could add commentary making that
clear, that would be very helpful.  I'm a little concerned that over time, the
content will morph as the code changes, but folks won't think to update this
comment; I wonder if it would be a good idea to put this comment next to the
code constructing it and just direct folks there from here?

>+    <menuitem id="copyNewsgroupAddressItem" label="&CopyNewsgroupAddress.label;"
>+              accesskey="&CopyNewsgroupAddress.accesskey;"
>+              oncommand="CopyNewsgroupAddress(findEmailNodeFromPopupNode(document.popupNode, 
'newsgroupPopup'))"/>

From an end-user standpoint, "newsgroup address" strikes me as a bit awkward-sounding and is not a phrase that I've ever heard used.  From a naive techie standpoint, I would expect it to put a news: URL on the clipboard.  How about changing this to "newsgroup name", both in the UI and in the function names?

>@@ -901,8 +901,9 @@ function HideMessageHeaderPane()
> 
> function OutputNewsgroups(headerEntry, headerValue)
> {
>-  headerValue = headerValue.replace(/,/g,", ");
>-  updateHeaderValue(headerEntry, headerValue);
>+  for each(let newsgroup in headerValue.split(","))

Nit: please add a space before the open paren.

hideEmailAddressPopup and hideNewsgroupPopup are identical.  How about just renaming 
hideEmailAddressPopup to something more generic and using it in both places?


>+function setupNewsgroupPopup(newsgroupNode)
>+{
>+    dump("newsgroupNode.tagName = " + newsgroupNode.tagName + '\n');

Please remove the dump(), if you would.  FWIW, I would support (in some other bug) moving log4moz out of the gloda dir into somewhere more central and adding generic logging to the header overlay stuff.  That would make the message header overly code significantly easier to debug, I suspect.

>+  let newsgroupPlaceHolder = document.getElementById('newsgroupPlaceHolder');
>+  let newsgroup = newsgroupNode.getAttribute('newsgroup');
>+  newsgroupNode.setAttribute('selected', 'true');
>+  newsgroupPlaceHolder.setAttribute('label', newsgroup);
>+
>+  let server = GetNewsgroupServer();
>+  if (server)
>+  {
>+    // XXX Why is this necessary when nsISubscribableServer contains |isSubscribed|?
>+    server = server.QueryInterface(Components.interfaces.nsINntpIncomingServer);

What happens when you try and access .isSubscribed?

>+function SubscribeToNewsgroup(newsgroupNode)
>+{
>+  let server = GetNewsgroupServer();
>+  if (server)
>+  {
>+    let newsgroup = newsgroupNode.getAttribute('newsgroup');
>+    server = server.QueryInterface(Components.interfaces.nsISubscribableServer);

Since GetNewsgroupServer() already did this exact QI, is it necessary to do it again here?

>+/**
>+ * Takes the newsgroup address title button, extracts the newsgroup address we stored
>+ * in there and opens a compose window with that address.
>+ *Â @param newsgroupNode a node which has a "newsgroup" attribute
>+ */
>+function SendMessageToNewsgroup(newsgroupNode)

This function is identical to SendMailToNode except for a single JS expression; this isn't worth a fork.  Please parameterize the existing function and rename it to something slightly more generic.  The same thing applies to CopyNewsgroupAddress.

I appreciate your willingness to wade into the header overlay code to tackle this!

Magnus Melin [:mkmelin]

Comment 32

•

15 years ago

(In reply to comment #31)
> From an end-user standpoint, "newsgroup address" strikes me as a bit
> awkward-sounding and is not a phrase that I've ever heard used.  From a naive
> techie standpoint, I would expect it to put a news: URL on the clipboard.  How
> about changing this to "newsgroup name", both in the UI and in the function
> names?

At least to me sounds quite useful to be able to copy the ng URL, while copying the name is far less useful. If "address" sounds odd, maybe "Copy URL" or "Copy Location"?

Assignee

Comment 33

•

15 years ago

Attached patch Address comments (obsolete) — Details — Splinter Review

(In reply to comment #31)
> What motivated the decision to create what's effectively a fork of
> <mail-multi-emailHeaderField> and <mail-emailaddress>(as opposed to
> generalizing and/or specializing the existing bindings)?  I'm not saying that
> the decision to fork is the wrong one, but it feels like there's a lot of
> duplicated code here, so I'd like to understand it better...

It was mostly that mail addresses have a lot of code for handling address books and updating based on changes to the address book, which is all irrelevant for newsgroups. A generic "mail-multiple-headerfield" as a basis for the email, newsgroup, and message-id headers could be useful, though. It would also make it easier to resolve bug 512036.

> For added functions/methods that aren't completely trivial, I'd like to request
> that you include detailed doxygen-style comments above them specifying
> behavior, parameters, and return values.  This significantly eases the lives of
> folks who want to use, read, or maintain the code.  It's particularly useful
> during maintenance, because it makes it possible to tell if specific behavior
> was intentional or a bug that should be fixed.
> As you've undoubtedly noticed, most of the code in this file does quite poorly
> at this, but I don't want code that we add to compound what's already a bad
> problem.

Done, I think. I've added standard comments to all the JS code, and some brief XML comments to all the XBL implementation methods. If the format for the XML comments is wrong, let me know.

> 
> >+  <binding id="mail-newsgroup-headerfield">
> 
> How about naming this "mail-newsgroups-headerfield"? I think that would be more
> clear/accurate to folks reading the code.

Done.

> >+    <content>
> >+      <xul:hbox class="headerNameBox" align="baseline">
> >+        <xul:label class="headerName" xbl:inherits="value=label" control="headerValue" flex="1"/>
> >+      </xul:hbox>
> >+
> >+      <xul:hbox class="headerValueBox" anonid="newsgroups" flex="1"
> >+                singleline="true"
> >+                align="baseline">
> >+      </xul:hbox>
> >+      <!--<xul:mail-newsgroup class="headerValue" anonid="newsgroupNode"/>-->
> 
> I assume the above comment is intended to be an example of simple anonymous
> content that gets built up by the XBL?  If you could add commentary making that
> clear, that would be very helpful.  I'm a little concerned that over time, the
> content will morph as the code changes, but folks won't think to update this
> comment; I wonder if it would be a good idea to put this comment next to the
> code constructing it and just direct folks there from here?

Nah, that's just a comment I forgot to remove! If you think a comment like that would be helpful, I can put it in some appropriate spot, but as it is, it's just another thing to get out of sync.

> >+    <menuitem id="copyNewsgroupAddressItem" label="&CopyNewsgroupAddress.label;"
> >+              accesskey="&CopyNewsgroupAddress.accesskey;"
> >+              oncommand="CopyNewsgroupAddress(findEmailNodeFromPopupNode(document.popupNode, 
> 'newsgroupPopup'))"/>
> 
> From an end-user standpoint, "newsgroup address" strikes me as a bit
> awkward-sounding and is not a phrase that I've ever heard used.  From a naive
> techie standpoint, I would expect it to put a news: URL on the clipboard.  How
> about changing this to "newsgroup name", both in the UI and in the function
> names?

Switched to newsgroup name. I also eliminated referencing a newsgroup name as an "address" in the XBL.

> >@@ -901,8 +901,9 @@ function HideMessageHeaderPane()
> > 
> > function OutputNewsgroups(headerEntry, headerValue)
> > {
> >-  headerValue = headerValue.replace(/,/g,", ");
> >-  updateHeaderValue(headerEntry, headerValue);
> >+  for each(let newsgroup in headerValue.split(","))
> 
> Nit: please add a space before the open paren.

Fixed.

> hideEmailAddressPopup and hideNewsgroupPopup are identical.  How about just
> renaming 
> hideEmailAddressPopup to something more generic and using it in both places?

Done.

> >+function setupNewsgroupPopup(newsgroupNode)
> >+{
> >+    dump("newsgroupNode.tagName = " + newsgroupNode.tagName + '\n');
> 
> Please remove the dump(), if you would.  FWIW, I would support (in some other
> bug) moving log4moz out of the gloda dir into somewhere more central and adding
> generic logging to the header overlay stuff.  That would make the message
> header overly code significantly easier to debug, I suspect.

Done.

> >+  let newsgroupPlaceHolder = document.getElementById('newsgroupPlaceHolder');
> >+  let newsgroup = newsgroupNode.getAttribute('newsgroup');
> >+  newsgroupNode.setAttribute('selected', 'true');
> >+  newsgroupPlaceHolder.setAttribute('label', newsgroup);
> >+
> >+  let server = GetNewsgroupServer();
> >+  if (server)
> >+  {
> >+    // XXX Why is this necessary when nsISubscribableServer contains |isSubscribed|?
> >+    server = server.QueryInterface(Components.interfaces.nsINntpIncomingServer);
> 
> What happens when you try and access .isSubscribed?

No matter what, it returns false.

> >+function SubscribeToNewsgroup(newsgroupNode)
> >+{
> >+  let server = GetNewsgroupServer();
> >+  if (server)
> >+  {
> >+    let newsgroup = newsgroupNode.getAttribute('newsgroup');
> >+    server = server.QueryInterface(Components.interfaces.nsISubscribableServer);
> 
> Since GetNewsgroupServer() already did this exact QI, is it necessary to do it
> again here?

It's not necessary. I removed it.

> >+/**
> >+ * Takes the newsgroup address title button, extracts the newsgroup address we stored
> >+ * in there and opens a compose window with that address.
> >+ *Â @param newsgroupNode a node which has a "newsgroup" attribute
> >+ */
> >+function SendMessageToNewsgroup(newsgroupNode)
> 
> This function is identical to SendMailToNode except for a single JS expression;
> this isn't worth a fork.  Please parameterize the existing function and rename
> it to something slightly more generic.  The same thing applies to
> CopyNewsgroupAddress.

Done and done. I've also made a couple other minor changes, most notably that the newsgroup names wrap correctly now when they can't fit on one line.

> I appreciate your willingness to wade into the header overlay code to tackle
> this!

No problem! Since the only UI change is "Copy Newsgroup Address" -> "Copy Newsgroup Name", can I pull the ui-review+ forward onto this patch? I'm not sure what the prevailing etiquette is...

Attachment #395992 - Attachment is obsolete: true

Attachment #398752 - Flags: review?(dmose)

Assignee

Comment 34

•

15 years ago

Attached patch Fix bitrot — Details — Splinter Review

Attachment #398752 - Attachment is obsolete: true

Attachment #400536 - Flags: review?(dmose)

Attachment #398752 - Flags: review?(dmose)

Comment 35

•

15 years ago

(In reply to comment #32)
>
> At least to me sounds quite useful to be able to copy the ng URL, while copying
> the name is far less useful. If "address" sounds odd, maybe "Copy URL" or "Copy
> Location"?

The reason that copying the name is necessary is that by linkifying, we're removing the ability to select and copy that's available with unlinkified text.

I'd agree that making a URL available to copy is also useful, but let's do that in another bug.

(In reply to comment #33)
> > >+    // XXX Why is this necessary when nsISubscribableServer contains |isSubscribed|?
> > >+    server = server.QueryInterface(Components.interfaces.nsINntpIncomingServer);
> >
> > What happens when you try and access .isSubscribed?
> 
> No matter what, it returns false.

Please file a separate bug on this, if you would.

> No problem! Since the only UI change is "Copy Newsgroup Address" -> "Copy
> Newsgroup Name", can I pull the ui-review+ forward onto this patch? I'm not
> sure what the prevailing etiquette is...

We should get clarkbw to re-review the new version, but since it's only a string change, I expect that to be quite straightforward.

Comment 36

•

15 years ago

Comment on attachment 400536 [details] [diff] [review]
Fix bitrot

Looks good; I made a few tweaks to fix some nits, which I'll upload momentarily.

>+
>+  <binding id="mail-newsgroup">

Bindings deserve comments too.  :-)  I added one here and also to <mail-newsgroups-headerfield>.
 
>+// take string of newsgroups separated by commas, split it
>+// into newsgroups and send them to the corresponding
>+// mail-newsgroups-headerfield element
> function OutputNewsgroups(headerEntry, headerValue)

Converted to doxygen format.

>+  for each (let newsgroup in headerValue.split(","))
>+    headerEntry.enclosingBox.addNewsgroupView(newsgroup);

In general, it's a good habit to prefer Array.forEach when working with arrays; see <https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in#Description> for the gory details.  Using that with an expression closure yields:

  headerValue.split(",").forEach(
    function(newsgroup) headerEntry.enclosingBox.addNewsgroupView(newsgroup));

>diff -r 415f1b9ca42a mail/themes/pinstripe/mail/messageHeader.css
>--- a/mail/themes/pinstripe/mail/messageHeader.css	Mon Sep 14 16:12:36 2009 +0100
>+++ b/mail/themes/pinstripe/mail/messageHeader.css	Mon Sep 14 12:37:40 2009 -0500
>@@ -478,8 +478,9 @@ mail-emailaddress[selected="true"] .emai
>   -moz-border-radius-bottomright: 0;
> }
> 
>-.emailSeparator {
>-  /* this is for the comma in between email addresses */
>+.emailSeparator,
>+.newsgroupSeparator {
>+  /* this is for the comma in between email addresses/newsgroups */
>   -moz-margin-start: -6px; /* squeeze it inside the bubble, by the star */

Because, unlike email addresses in the header, there's no star next to newsgroup names, using the same -moz-margin-start pulls the comma back so far that it renders on top of the last letter of the newsgroup name, which looks bad, so I split this out into a separate rule.  It seems to be significantly more noticeable on Mac; I'm in the process of testing on Windows and Linux to see whether they want similar changes.

Attachment #400536 - Flags: review?(dmose) → review+

Comment 37

•

15 years ago

Attached patch updated patch with review nits picked (obsolete) — Details — Splinter Review

Updated patch; thanks for putting this together, Jim!

Bryan, you've already done the main UI-review, this re-request is just asking for an OK on the wording change I've proposed for the menu item.  It used to be "Copy Newsgroup Address", it's now "Copy Newsgroup Name", reasoning pasted from comment 31:

From an end-user standpoint, "newsgroup address" strikes me as a bit
awkward-sounding and is not a phrase that I've ever heard used.  From a naive
techie standpoint, I would expect it to put a news: URL on the clipboard.  How
about changing this to "newsgroup name", both in the UI and in the function
names?

Attachment #401151 - Flags: ui-review?(clarkbw)

Attachment #401151 - Flags: review+

Comment 38

•

15 years ago

Attached patch Updated patch with similar CSS tweak for commas on Windows & Linux — Details — Splinter Review

Attachment #401151 - Attachment is obsolete: true

Attachment #401231 - Flags: ui-review?(clarkbw)

Attachment #401231 - Flags: review+

Attachment #401151 - Flags: ui-review?(clarkbw)

Comment 39

•

15 years ago

Comment on attachment 401231 [details] [diff] [review]
Updated patch with similar CSS tweak for commas on Windows & Linux

The wording and arrangement of options looks good so I think we could land this.

The only thing I didn't like was the lack of feedback from subscribing to a newsgroup.  (this could be a different bug or followup patch)

When I selected "subscribe" it happened so fast that I wasn't sure what it really did.  (the new newsgroup happened to appear below the scroll of the folder pane too)  I don't think we want to swap people over to that newsgroup but it would be good to see some kind of indication.

Attachment #401231 - Flags: ui-review?(clarkbw) → ui-review+

Comment 40

•

15 years ago

Patch checked in: <http://build.mozillamessaging.com/mercurial/comm-central/rev/d0e6ed7c9ca1>.  Jim, can you spin off a new bug for a visual transition as suggested in comment 39?  Thanks!

Status: ASSIGNED → RESOLVED

Closed: 21 years ago → 15 years ago

Resolution: --- → FIXED

Assignee

Updated

•

15 years ago

Blocks: 517373

Assignee

Comment 41

•

15 years ago

I filed bug 517373 for the "subscribe to newsgroup" feedback.

I'm going to check a couple things about the nsISubscribableServer.isSubscribed bug before filing anything on it, but hopefully I'll get to that tomorrow.

No longer blocks: 517373

Assignee

Updated

•

15 years ago

Blocks: 517373

Joe Sabash [:JoeS1]

Comment 42

•

15 years ago

Does this also fix linking to a specific newsgroup message (I hope)