Closed Bug 492329 Opened 15 years ago Closed 7 years ago

Polish the error message for an expired newsgroup article

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Windows Vista
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 56.0

People

(Reporter: wladow, Assigned: jorgk-bmo)

References

Details

(Keywords: polish)

Attachments

(4 files, 10 obsolete files)

10.55 KB, image/png
Details
14.55 KB, image/png
Details
435.85 KB, text/plain
Details
32.58 KB, patch
jorgk-bmo
: review+
infofrommozilla
: feedback+
Details | Diff | Splinter Review
Attached image 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?
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
Severity: normal → trivial
In which file is this message generated?
<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.
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
(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.
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)
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.
(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.
(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.
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.
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+
Attached patch de-bitrotted patch (obsolete) — Splinter Review
Attachment #576695 - Attachment is obsolete: true
Attachment #576695 - Flags: superreview?(neil)
Attachment #576695 - Flags: review?(dbienvenu)
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-
nsNntpService::HandleContent is canceling the url - I'll poke around a bit.
(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.
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.
Attached patch fix list-ids (obsolete) — Splinter Review
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?
(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.
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?
Attached patch patch updated to tip (obsolete) — Splinter Review
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)
Resetting assignee since I have no immediate plans of working on this.
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(squibblyflabbetydoo)
Attached patch patch updated again (obsolete) — Splinter Review
Updated to apply on tip.
Attachment #8577755 - Attachment is obsolete: true
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?
(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.
Attached patch article-not-found.patch (obsolete) — Splinter Review
Rebased.
Attachment #8704505 - Attachment is obsolete: true
Attached patch article-not-found.patch (JK v2) (obsolete) — Splinter Review
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.
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
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)
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 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+
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 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: