Currently NNTP auth dialog just states "Please enter username (password) for news access". It should at least give the server name (as in Netscape 4.x_ and when doing per-newsgroup authentication, it should give the newsgroup name as well. P.S. A similar request for HTTP auth dialogs is known as bug #38008

This bug has a potential to be very annoying - when you have several newsservers being biffed and suddenly one decides to ask for password, you have no idea what went wrong and what server is doing this!

This patch mostly works, but needs minor fixes (for example, we need to have a special case when we are asking for a per-server auth info as opposed to per-group auth info). Can people take a look and say whether they agree with this patch in general and whether I got the strings magic fugured out correctly. Thanks!

Please post a screenshot of it in action

Wow, this is cool! One nit, before I review the patch: the alert shouldn't have a title - at all.

Known issues so far: - I tried to add a title to the username dialog (it does not currently have one), but it didn't work for some reason. - strings magic: my call to Right does not do the correct thing, it turns "/local.test" into "local.testt" instead of "local.test", what's the correct thing to do? P.S. Password dialogs have corrupted titles, this is a known KDE issue - see bug 9449 and bug 92150.

Do you mean - remove the title from the password prompt? It used to be there before... IMHO, the titles are good - they make it easier to see what's going on by looking at a window list in WM.

You shouldn't use a title, because the alert will/should be modal anyway. It also introduces problems, as you describe. Let's leave it out for now.

I removed the titles for now and I filed bug 95921 for a general discussion of whether auth dialogs should have titles. I still need help with calling Right the right way: > strings magic: my call to Right does not do the correct thing, it turns > "/local.test" into "local.testt" instead of "local.test", what's the correct > thing to do?

Hakan, the patch 46347 does not create titles, as you requested. Can you review it pls (I've been running with it for a while without problems)? Thanks!

Can somebody review attachment 46347 [details] [diff] [review], please?

Comment on attachment 46347 [details] [diff] [review] no-titles version Here are some nits. >Index: mailnews/news/src/nsNewsFolder.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/news/src/nsNewsFolder.cpp,v >retrieving revision 1.194 >diff -ILinux*.OBJ -u -r1.194 nsNewsFolder.cpp >--- mailnews/news/src/nsNewsFolder.cpp 2001/07/25 07:53:00 1.194 >+++ mailnews/news/src/nsNewsFolder.cpp 2001/08/18 15:36:30 >@@ -1358,10 +1358,53 @@ > return rv; > } > >+nsresult nsMsgNewsFolder::GetPromptTitleAndText(const PRUnichar *aTextName, >+ const PRUnichar *aTextName2, >+ PRUnichar **aText) >+{ Please be consistent with all other nsMsgNewsFolder methods and use const char* as in-parameters. >+ nsresult rv = NS_OK; >+ >+ nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIStringBundle> bundle; >+ rv = bundleService->CreateBundle(NEWS_MSGS_URL, getter_AddRefs(bundle)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIURL> url; >+ nsComponentManager::CreateInstance(kStandardUrlCID, nsnull, NS_GET_IID(nsIURL), (void **) getter_AddRefs(url)); >+ >+ rv = url->SetSpec(mURI); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsXPIDLCString host, group; >+ rv = url->GetHost(getter_Copies(host)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = url->GetPath(getter_Copies(group)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ PRUnichar * uhost = ToNewUnicode(host); In general, prefer nsXPIDLString to "raw" PRUnichar*s since you don't have to free the former manually. >+ >+ PRUint32 len = group.Length(); >+ >+ if (len > 1) { >+ group.Right(group,len-1); >+ PRUnichar * ugroup = ToNewUnicode(group); Is this really necessary? Why are you doing .Right() here? If you could avoid that copy from group -> ugroup, that would be good.

Comment on attachment 46347 [details] [diff] [review] no-titles version >Index: mailnews/news/src/nsNewsFolder.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/news/src/nsNewsFolder.cpp,v >retrieving revision 1.194 >diff -ILinux*.OBJ -u -r1.194 nsNewsFolder.cpp >--- mailnews/news/src/nsNewsFolder.cpp 2001/07/25 07:53:00 1.194 >+++ mailnews/news/src/nsNewsFolder.cpp 2001/08/18 15:36:30 >@@ -1358,10 +1358,53 @@ > return rv; > } > >+nsresult nsMsgNewsFolder::GetPromptTitleAndText(const PRUnichar *aTextName, >+ const PRUnichar *aTextName2, >+ PRUnichar **aText) >+{ >+ nsresult rv = NS_OK; >+ >+ nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIStringBundle> bundle; >+ rv = bundleService->CreateBundle(NEWS_MSGS_URL, getter_AddRefs(bundle)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIURL> url; >+ nsComponentManager::CreateInstance(kStandardUrlCID, nsnull, NS_GET_IID(nsIURL), (void **) getter_AddRefs(url)); nsCOMPtr<nsIURL> url(do_CreateInstance(NS_STANDARDURL_CONTRACTID)); >+ >+ rv = url->SetSpec(mURI); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsXPIDLCString host, group; >+ rv = url->GetHost(getter_Copies(host)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = url->GetPath(getter_Copies(group)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ PRUnichar * uhost = ToNewUnicode(host); >+ >+ PRUint32 len = group.Length(); >+ >+ if (len > 1) { >+ group.Right(group,len-1); >+ PRUnichar * ugroup = ToNewUnicode(group); >+ const PRUnichar *stringArray[] = { ugroup, uhost }; >+ rv = bundle->FormatStringFromName(aTextName, stringArray, 2, aText); >+ nsMemory::Free(ugroup); >+ } else { >+ const PRUnichar *stringArray[] = { uhost }; >+ rv = bundle->FormatStringFromName(aTextName2, stringArray, 1, aText); >+ } >+ >+ nsMemory::Free(uhost); >+ >+ return(rv); >+} >+ // can a group contain just "/" ? if (group.Length() > 1) { const PRUnichar *stringArray[] = { group.get() + 1, host.get() }; rv = bundle->FormatStringFromName(aTextName, stringArray, 2, aText); } else { const PRUnichar *stringArray[] = { host.get() }; rv = bundle->FormatStringFromName(aTextName2, stringArray, 1, aText); } >@@ -1405,9 +1448,15 @@ > rv = CreateNewsgroupPasswordUrlForSignon(mURI, getter_Copies(signonURL)); > if (NS_FAILED(rv)) return rv; > >- rv = dialog->PromptPassword(aPromptTitle, aPromptMessage, NS_ConvertASCIItoUCS2(NS_STATIC_CAST(const char*, signonURL)).get(), nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY, >+ nsXPIDLString passwordPromptText; >+ rv = GetPromptTitleAndText(NS_LITERAL_STRING("enterPassword").get(), This name is now incorrect. >+ NS_LITERAL_STRING("enterPassword2").get(), >+ getter_Copies(passwordPromptText)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = dialog->PromptPassword(nsnull, passwordPromptText, NS_ConvertASCIItoUCS2(NS_STATIC_CAST(const char*, signonURL)).get(), nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY, > getter_Copies(uniGroupPassword), &okayValue); Could you replace NS_ConvertASCIItoUCS2(NS_STATIC_CAST(const char*, signonURL)).get() with NS_ConvertUTF8toUCS2(signonURL).get() ? >- if (NS_FAILED(rv)) return rv; >+ NS_ENSURE_SUCCESS(rv, rv); Just keep this the way it was (no hidden returns). >@@ -1428,9 +1477,7 @@ > } > > NS_IMETHODIMP >-nsMsgNewsFolder::GetGroupUsernameWithUI(const PRUnichar * aPromptMessage, const >- PRUnichar *aPromptTitle, >- nsIMsgWindow* aMsgWindow, >+nsMsgNewsFolder::GetGroupUsernameWithUI(nsIMsgWindow* aMsgWindow, > char **aGroupUsername) "While you're there..." could you indent the second line so it lines up? > { > nsresult rv = NS_ERROR_FAILURE;; >@@ -1471,11 +1518,17 @@ > nsXPIDLCString signonURL; > rv = CreateNewsgroupUsernameUrlForSignon(mURI, getter_Copies(signonURL)); > if (NS_FAILED(rv)) return rv; >- >- rv = dialog->Prompt(aPromptTitle, aPromptMessage, NS_ConvertASCIItoUCS2(NS_STATIC_CAST(const char*, signonURL)).get(), >+ >+ nsXPIDLString usernamePromptText; >+ rv = GetPromptTitleAndText(NS_LITERAL_STRING("enterUsername").get(), >+ NS_LITERAL_STRING("enterUsername2").get(), >+ getter_Copies(usernamePromptText)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = dialog->Prompt(nsnull, usernamePromptText, NS_ConvertASCIItoUCS2(NS_STATIC_CAST(const char*, signonURL)).get(), Here too use UTF8toUCS2. > nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY, nsnull, > getter_Copies(uniGroupUsername), &okayValue); >- if (NS_FAILED(rv)) return rv; >+ NS_ENSURE_SUCCESS(rv, rv); Same comment applies.

So ignore my comment about providing a title and just rename the function (I just read bug 95921).

I am no longer that interested in this, so I am not sure when (if) I will have time to clean up my patch. Please feel free to take over.

sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.

Filter on "Nobody_NScomTLD_20080620"

Password issue; CC'ing Standard8 so he's aware that this bug exists, and can mark WONTFIX if it turns out to be impractical. Patch shouldn't be too bitrotted, but the string usage is out of date and inconsistent.