Polish the error message for an expired newsgroup article

RESOLVED FIXED in Thunderbird 56.0

Status

Thunderbird
Mail Window Front End
--
trivial
RESOLVED FIXED
9 years ago
5 months ago

People

(Reporter: wladow, Assigned: Jorg K (GMT+1) [currently bustage-fix only, no NI? or r?])

Tracking

({polish})

Trunk
Thunderbird 56.0
x86
Windows Vista
polish

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

10.55 KB, image/png
Details
14.55 KB, image/png
Details
435.85 KB, text/plain
Details
32.58 KB, patch
Jorg K (GMT+1) [currently bustage-fix only, no NI? or r?]
: review+
Alfred Peters
: feedback+
Details | Diff | Splinter Review
Created attachment 376687 [details]
screenshot

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090510 Shredder/3.0b3pre

When there is an expired newsgroup article, Shredder sets up the error message by joining 2 sentenses together, but with no space between them:

Error!
newsgroup server responded:no such article in group
                  ^^^^^^^^^^^
Flags: wanted-thunderbird3?
Created attachment 377227 [details]
article not found mockup

If someone is going to work on this I'd suggest some other changes as well.

Start with the Firefox 'problem loading page' and clean this whole thing up.  (see attached)
Summary: Polish the error message for an expired newsgroup arctile → Polish the error message for an expired newsgroup article

Updated

9 years ago
Severity: normal → trivial
In which file is this message generated?

Comment 3

6 years ago
Richard: http://mxr.mozilla.org/comm-central/search?string=htmlNewsError
<http://mxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp?mark=1990-2052#1989> contains the source code for the construction of the HTML, with some of the contents for strings being found in localized properties files.
Unfortunately c++ is totally out of my possibilities.

I think if "chrome://global/skin/netError.css" could be included here as stylesheet, this could already give the needed polish without other big changes.

Comment 6

6 years ago
This would be nice. Question: should we also go ahead and make this page work like about:neterror[1]? That would probably be a step up from generating HTML in C++.

[1] http://mxr.mozilla.org/comm-central/source/mozilla/docshell/resources/content/netError.xhtml

Comment 7

6 years ago
(In reply to Jim Porter (:squib) from comment #6)
> This would be nice. Question: should we also go ahead and make this page
> work like about:neterror[1]? That would probably be a step up from
> generating HTML in C++.
Sure, seems reasonable to me.

Comment 8

6 years ago
Created attachment 576695 [details] [diff] [review]
Prettify the "article not found" page

Here's a patch to do this. The result should be identical to the mockup (except for no bold text on the server response).
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #576695 - Flags: ui-review?(bwinton)
Attachment #576695 - Flags: superreview?(neil)
Attachment #576695 - Flags: review?(dbienvenu)

Comment 9

6 years ago
Oh, I couldn't test the "Remove expired articles" button, since it didn't seem to do anything before the patch either... if someone else can get it to work, it's probably worth testing just to make sure it still works (content policy might bite us).
Comment on attachment 576695 [details] [diff] [review]
Prettify the "article not found" page

Is there an easy way to test this?

>+          document.getElementById("errorTryAgain").hidden = true;
hidden doesn't work in HTML.

>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>+                       escapedResponse))
Please use MsgEscapeURL from nsMsgUtils.h

>+      uri.Trim("&", false, true, false);
Please can you use the 3-argument version of Trim.

Comment 11

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #10)
> Is there an easy way to test this?

The only way I have to test it is from some articles on mozilla.support.thunderbird, which is actually what prompted me to fix this. It's probably not super helpful, but if you filter on messages by "Ron Hunter", there should be a few recent articles that trigger this. Specifically, "Re: Attaching a shortcut to an email", ID <b-udnUg4tbiuNlHTnZ2dnUVZ_tednZ2d@mozilla.org> works for me to test.

> >+          document.getElementById("errorTryAgain").hidden = true;
> hidden doesn't work in HTML.

It's listed in the global attributes for HTML5 (granted, this page is XHTML, so I might be violating the spec anyway): https://developer.mozilla.org/en/HTML/Global_attributes

> >+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
> >+                       escapedResponse))
> Please use MsgEscapeURL from nsMsgUtils.h

Is there a reason this is preferred over NS_EscapeURL?

> >+      uri.Trim("&", false, true, false);
> Please can you use the 3-argument version of Trim.

I assume that's the same, but just without the last argument?

Two other things:

1) I noticed that I forgot to add the about: redirection for Seamonkey, so I'll need to
   do that.
2) There appear to be only two other places that DisplayHTMLInMessagePane is used (SMIME
   decryption errors and no cached bodies in offline mode). Maybe we should fix these
   while we're here too. I could either a) make separate files for them, or b) move
   newsError.xhtml to mailnewsError.xhtml and incorporate those errors into that page.
   Then we could deprecate and ultimately remove DisplayHTMLInMessagePane (which just
   loads a data: URI anyway).
(In reply to Jim Porter from comment #11)
> (In reply to from comment #10)
> > Is there an easy way to test this?
> The only way I have to test it is from some articles on
> mozilla.support.thunderbird, which is actually what prompted me to fix this.
> It's probably not super helpful, but if you filter on messages by "Ron
> Hunter", there should be a few recent articles that trigger this.
> Specifically, "Re: Attaching a shortcut to an email", ID
> <b-udnUg4tbiuNlHTnZ2dnUVZ_tednZ2d@mozilla.org> works for me to test.
Hmm, but if it's expired, then I won't have it in my message list, will I?

> > >+          document.getElementById("errorTryAgain").hidden = true;
> > hidden doesn't work in HTML.
> It's listed in the global attributes for HTML5
Aha, it was added by bug 567663. Thanks for pointing that out.

> > >+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
> > >+                       escapedResponse))
> > Please use MsgEscapeURL from nsMsgUtils.h
> Is there a reason this is preferred over NS_EscapeURL?
Yes, it works with --with-libxul-sdk.

> > >+      uri.Trim("&", false, true, false);
> > Please can you use the 3-argument version of Trim.
> I assume that's the same, but just without the last argument?
Yes, except when using --with-libxul-sdk which only supports three arguments.

> Then we could deprecate and ultimately remove DisplayHTMLInMessagePane
Sounds like a followup bug.

Comment 13

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #12)
> (In reply to Jim Porter from comment #11)
> > (In reply to from comment #10)
> > > Is there an easy way to test this?
> > The only way I have to test it is from some articles on
> > mozilla.support.thunderbird, which is actually what prompted me to fix this.
> > It's probably not super helpful, but if you filter on messages by "Ron
> > Hunter", there should be a few recent articles that trigger this.
> > Specifically, "Re: Attaching a shortcut to an email", ID
> > <b-udnUg4tbiuNlHTnZ2dnUVZ_tednZ2d@mozilla.org> works for me to test.
> Hmm, but if it's expired, then I won't have it in my message list, will I?

I'll attach my .msf file for the newsgroup. That should hopefully make it possible to test this.

> > Then we could deprecate and ultimately remove DisplayHTMLInMessagePane
> Sounds like a followup bug.

Fair enough.

Comment 14

6 years ago
Created attachment 577124 [details]
My .msf file for m.s.thunderbird

Here's an .msf file which should have an expired article. Filter on "Ron Hunter attachment" with the quick filter (on subject/sender) to find the expired article.

Comment 15

6 years ago
Hi. Could you please file the followup bug for removing DisplayHTMLInMessagePane . It supposedly would fix bug 57115. Thanks.
Comment on attachment 576695 [details] [diff] [review]
Prettify the "article not found" page

I mostly like it, but I've got a couple of questions.

For reference, there's a screenshot of the patch in action at http://dl.dropbox.com/u/2301433/Glass/MissingNewsgroupArticle.png

1) Why not bold the server response?
2) Now that we've landed OpenSearch, I think it would be nice for the message id after "Try searching for article: " to be a link to actually search for that article.

But even without those, it's a big step forward, so ui-r=me.

Thanks,
Blake.
Attachment #576695 - Flags: ui-review?(bwinton) → ui-review+

Comment 17

6 years ago
Created attachment 597623 [details] [diff] [review]
de-bitrotted patch
Attachment #576695 - Attachment is obsolete: true
Attachment #576695 - Flags: superreview?(neil)
Attachment #576695 - Flags: review?(dbienvenu)

Comment 18

6 years ago
Comment on attachment 597623 [details] [diff] [review]
de-bitrotted patch

nit - no space after nsCOMPtr
+  nsCOMPtr <nsIDocShell> docShell;
+  GetMessageWindowDocShell(getter_AddRefs(docShell));

you could pass &rv to do_QueryInterface and return the actual rv instead of NS_ERROR_FAILURE with NS_ENSURE_SUCCESS(rv, rv);

+  nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(docShell));
+  NS_ENSURE_TRUE(webNav, NS_ERROR_FAILURE);

I tried this in TB and it is a nice improvement. Removing expired articles button didn't work, however, which is why I've minused this. We start the url, but we never get into the nntp protocol state machine. I don't think content policy is kicking us out (which is usually the issue with things like this), but I'm not sure why we're not getting into the state machine. Did it work for you?
Attachment #597623 - Flags: superreview?(neil)
Attachment #597623 - Flags: review-

Comment 19

6 years ago
nsNntpService::HandleContent is canceling the url - I'll poke around a bit.

Comment 20

6 years ago
(In reply to David :Bienvenu from comment #18)
> I tried this in TB and it is a nice improvement. Removing expired articles
> button didn't work, however, which is why I've minused this. We start the
> url, but we never get into the nntp protocol state machine. I don't think
> content policy is kicking us out (which is usually the issue with things
> like this), but I'm not sure why we're not getting into the state machine.
> Did it work for you?

It doesn't, but it never worked before either, so I figured I was just doing something wrong (as a user). Maybe not.

Comment 21

6 years ago
the code broke at some point, nothing at all to do with your patch from what I can tell. I'll fix it and attach a qrefreshed patch.

Comment 22

6 years ago
Created attachment 597871 [details] [diff] [review]
fix list-ids

this gets the button working for me. I think jcranmer regressed this with his newsuri fix
Attachment #597623 - Attachment is obsolete: true
Attachment #597871 - Flags: superreview?(neil)
Attachment #597623 - Flags: superreview?(neil)
Comment on attachment 597871 [details] [diff] [review]
fix list-ids

>+      // this is all we need for listing newsgroup ids, I think.
>+      if (!PL_strcasecmp(aContentType, "x-application-newsgroup-listids"))
>+        return rv;
Does this need to go on branches? If so maybe a separate bug?

Comment 24

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #23)
> Comment on attachment 597871 [details] [diff] [review]
> fix list-ids
> 
> >+      // this is all we need for listing newsgroup ids, I think.
> >+      if (!PL_strcasecmp(aContentType, "x-application-newsgroup-listids"))
> >+        return rv;
> Does this need to go on branches? If so maybe a separate bug?

Yeah, I think the regression is on one or more branches. OK, filed bug 727951
Comment on attachment 597871 [details] [diff] [review]
fix list-ids

>+      nsString uri(NS_LITERAL_STRING("about:newserror?"));
>+      uri.AppendLiteral("r=");
about:newserror?r=
Might be worth putting this in an nsCString instead, makes the appends easier.

>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>+                       escapedResponse))
>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>+      else
>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
etc. please.

>+      uri.AppendLiteral("&");
Don't do this here, you might not need it.

>         rv = m_newsFolder->GetMessageIdForKey(m_key, messageID);
>-        if (NS_SUCCEEDED(rv)) {
Why assign rv if you're not checking it?

>+        uri.AppendLiteral("m=");
Instead, append &m= here etc.

>+      uri.Trim("&", false, true, false);
Then this becomes unnecessary.
(In reply to comment #25)
>(From update of attachment 597871 [details] [diff] [review])
>>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>>+                       escapedResponse))
>>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>>+      else
>>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
>MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
Should be ESCAPE_URL_QUERY of course.
(In reply to comment #25)
>(From update of attachment 597871 [details] [diff] [review])
>>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>>+                       escapedResponse))
>>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>>+      else
>>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
>MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
Not my day, is it? This needs to be nsDependentCString(m_responseText), but the other two cases are OK because they are already nsCString objects.

Comment 28

6 years ago
I think I may have a local patch with some of these changes that I neglected to post, since I got busy with BigFiles. We'll see when I'm at my dev box...
(In reply to comment #25)
>(From update of attachment 597871 [details] [diff] [review])
>>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>>+                       escapedResponse))
>>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>>+      else
>>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
>MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
Should be MsgEscapeURL of course. Oops.
Comment on attachment 597871 [details] [diff] [review]
fix list-ids

>+    "newserror": {url: "chrome://messenger/content/newsError.xhtml",
>+                flags: Ci.nsIAboutModule.ALLOW_SCRIPT},
Hmm, I guess we should implement something like this; currently we have eight separate about handler files, so this would be the ninth... although there is something to be said about having this handler in a shared location.

>+<!ENTITY articleNotFound.desc "The newsgroup server reports that it can't find the article">
Needs trailing full stop.

>+<!ENTITY articleExpired.title "Perhaps the article has expired">
Needs trailing question mark.
Comment on attachment 597871 [details] [diff] [review]
fix list-ids

Cancelling review request - from what I can tell in Neil's comments, this patch needs some updates before we can move forward.
Attachment #597871 - Flags: superreview?(neil)
(In reply to comment #30)
>(From update of attachment 597871 [details] [diff] [review])
>>+    "newserror": {url: "chrome://messenger/content/newsError.xhtml",
>>+                flags: Ci.nsIAboutModule.ALLOW_SCRIPT},
>Hmm, I guess we should implement something like this; currently we have
>eight separate about handler files, so this would be the ninth... although
>there is something to be said about having this handler in a shared location.
SeaMonkey has now switched to a single nsAbout.js for all of its handlers, so it would be helpful if you could include it in your next patch. Thanks.
Flags: wanted-thunderbird3?
Created attachment 8577755 [details] [diff] [review]
patch updated to tip

I've updated the to tip and it is still working. :)

Jim, if you have time, please could you look at Neil's comments and finish this patch? IMHO I think it is not so much to do to finish it.
Attachment #597871 - Attachment is obsolete: true
Adding n-i.
Flags: needinfo?(squibblyflabbetydoo)

Comment 35

3 years ago
Resetting assignee since I have no immediate plans of working on this.
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(squibblyflabbetydoo)
Created attachment 8704505 [details] [diff] [review]
patch updated again

Updated to apply on tip.
Attachment #8577755 - Attachment is obsolete: true

Comment 37

2 years ago
Patch 8704505 results in a compiler error on Linux:

https://pastebin.mozilla.org/8855953

PRUnichar is uint16_t on Linux which is a different type than char16_t, of course. Which one is the right one here?

Comment 38

2 years ago
(In reply to Lars Rohwedder from comment #37)
> PRUnichar is uint16_t on Linux which is a different type than char16_t, of
> course. Which one is the right one here?

You could look at the definition of webNav->LoadURI to find out what is expected here.
Created attachment 8879855 [details] [diff] [review]
article-not-found.patch

Rebased.
Attachment #8704505 - Attachment is obsolete: true
Created attachment 8879901 [details] [diff] [review]
article-not-found.patch (JK v2)

Made it compile.
Attachment #8879855 - Attachment is obsolete: true
On an expired article the current patch shows an empty screen with this error in the error console:

Security Error: Content at moz-nullprincipal:{dd24085b-bc16-4419-af75-e1caa8a7de6c} may not load or link to about:newserror?r=no%20such%20article%20in%20group%20(3451%203452-3460)&m=mailman.82.1477392692.16819.test-multimedia@lists.mozilla.org&f=news://news.mozilla.org/mozilla.test.multimedia.

That error URL needs to be loaded with a system principal, updated patch coming.
Created attachment 8880931 [details] [diff] [review]
492329-article-not-found.patch (JK v3)

This is working now. I've shuffled the principals around a bit.

I clicked on the link and the expired articles disappeared, so working as designed.
Assignee: nobody → jorgk
Attachment #8879901 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Created attachment 8880939 [details] [diff] [review]
492329-article-not-found.patch (JK v4)

OK, I've addressed the nits from comment #25, so as far as I can see, this is ready to go.

Sadly, I have blown away all my expired articles, so I can't test it any more. Alfred, can you please test this for me.
Attachment #8880931 - Attachment is obsolete: true
Attachment #8880939 - Flags: review+
Attachment #8880939 - Flags: feedback?(infofrommozilla)
Created attachment 8880948 [details] [diff] [review]
492329-article-not-found.patch (JK v4b).

Oops, cut&paste error.

Alfred, maybe you can also look over the code to see whether you can spot any problems.
Attachment #8880939 - Attachment is obsolete: true
Attachment #8880939 - Flags: feedback?(infofrommozilla)
Attachment #8880948 - Flags: review+
Attachment #8880948 - Flags: feedback?(infofrommozilla)

Comment 45

5 months ago
Comment on attachment 8880948 [details] [diff] [review]
492329-article-not-found.patch (JK v4b).

I do not see any obvious problems.
Work as intended.

Just a nit:

We fetch the articles via the article number (m_key).
My server reports accordingly: "no such article number in this group"

So far we have the number also reported.

| -    PR_snprintf(outputBuffer, OUTPUT_BUFFER_SIZE,"<P>&lt;%.512s&gt; (%lu)", messageID.get(), m_key);
Attachment #8880948 - Flags: feedback?(infofrommozilla) → feedback+
Created attachment 8881028 [details] [diff] [review]
492329-article-not-found.patch (JK v5)

Alfred, thanks for looking at this. This version should now satisfy the most demanding news reader ;-)
Attachment #8880948 - Attachment is obsolete: true
Attachment #8881028 - Flags: review+
Attachment #8881028 - Flags: feedback?(infofrommozilla)

Comment 47

5 months ago
Comment on attachment 8881028 [details] [diff] [review]
492329-article-not-found.patch (JK v5)

Excellent!

For the normal user, the information is of no interest.
But, if the message ID index of the server is broken, 'ARTICLE <m_key>' and ARTICLE <message-id>' can produce different results.
Attachment #8881028 - Flags: feedback?(infofrommozilla) → feedback+
https://hg.mozilla.org/comm-central/rev/f3926c01ebe5f4796bfbe696101c7b13e4b44e88
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0

Updated

5 months ago
Depends on: 1377659
You need to log in before you can comment on or make changes to this bug.