Open Bug 226956 Opened 18 years ago Updated 7 years ago

Bug 197315, </mailnews/*> files: Convert <window class="dialog"> to <dialog>

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
trivial

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

See bug 197315 "ToDoList" attachment:
there are +/- 15 <.xul> files to process.
Status: NEW → ASSIGNED
Attached patch (Av1) <sendProgress.*> (obsolete) — Splinter Review
Comment on attachment 136517 [details] [diff] [review]
(Av1) <sendProgress.*>


'r=?':
Can you review, test it ?

I wonder if |function getString(stringId)| could be simplyfied:
*removing the try+catch
*or removing the if+else
?
Attachment #136517 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 136517 [details] [diff] [review]
(Av1) <sendProgress.*>

>-  <script type="application/x-javascript" src="chrome://messenger/content/messengercompose/sendProgress.js"/>
>+  <script type="application/x-javascript"
>+          src="chrome://messenger/content/messengercompose/sendProgress.js"/>
Please don't randomly wrap lines.

>-var itsASaveOperation = false;
>+var itIsASaveOperation = false;
"it's a save operation" -> remove apostrophes, remove spaces but upper case the
next letter -> "itsASaveOperation"

>-   if (!(stringId in dialog.strings)) {
>+  if (!(stringId in dialog.strings))
>+  {
Please don't change brace style.

>-        dump( "Invalid argument to downloadProgress.xul\n" );
>+    dump("Invalid argument to sendProgress.xul\n");
Please don't randomly change whitespace style.

Where I say "randomly", this means somewhere where you don't already have to
change the line. Even when you do have to change the line, don't change brace
style unless it's inconsistent with the rest of the file. The rest looks OK.
Attachment #136517 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch (Av2) <sendProgress.*> (obsolete) — Splinter Review
Patch v1, plus comment 3 suggestions.
Attachment #136517 - Attachment is obsolete: true
Attachment #136632 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #136517 - Attachment description: <sendProgress.*> patch v1_Bw (for review only) → <sendProgress.*> patch v1_Bw
Attachment #136517 - Attachment description: <sendProgress.*> patch v1_Bw → <sendProgress.*> patch v1
Attachment #136632 - Attachment is obsolete: true
Attachment #136632 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch (Av2b) <sendProgress.*> (obsolete) — Splinter Review
Patch v2, plus full "random" line unwrapping :->
Attachment #136689 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #136689 - Attachment is obsolete: true
Attachment #136689 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch (Av2c) <sendProgress.*> (obsolete) — Splinter Review
Patch v2b, plus a 'downloadProgress' to 'sendProgress' renaming !
Attachment #136690 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 136690 [details] [diff] [review]
(Av2c) <sendProgress.*>

>+<!DOCTYPE dialog
>+  SYSTEM "chrome://messenger/locale/messengercompose/sendProgress.dtd">
You don't need to unwrap this for just one doctype.

>+>
Please don't do this.
Attachment #136690 - Flags: review?(neil.parkwaycc.co.uk) → review-
Patch v2c, plus comment 7 suggestions.
Attachment #136690 - Attachment is obsolete: true
Attachment #136897 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #136690 - Attachment description: <sendProgress.*> patch v2c → (Av2c) <sendProgress.*>
Attachment #136517 - Attachment description: <sendProgress.*> patch v1 → (Av1) <sendProgress.*>
Attachment #136632 - Attachment description: <sendProgress.*> patch v2 → (Av2) <sendProgress.*>
Attachment #136689 - Attachment description: <sendProgress.*> patch v2b → (Av2b) <sendProgress.*>
Attachment #136897 - Attachment description: <sendProgress.*> patch v3 → (Av3) <sendProgress.*>
Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]

Actually I've since discovered that other people prefer the styles that I
carped at in your previous patch.
Attachment #136897 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]


'approval1.6=?': Trivial U.I. code cleanup.
Attachment #136897 - Flags: superreview?(bienvenu)
Attachment #136897 - Flags: approval1.6?
Attachment #136897 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]

need to minus.	trying not to take changes that impact	localizations after
beta.  let's do this for 1.7
Attachment #136897 - Flags: approval1.6? → approval1.6-
Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]


Check in: { 12/20/2003 10:13	neil%parkwaycc.co.uk }
Attachment #136897 - Attachment description: (Av3) <sendProgress.*> → (Av3) <sendProgress.*> [Checked in: Comment 12]
Attachment #136897 - Attachment is obsolete: true
Depends on: 242454
Serge, are you currently working on this? Maybe I could help a little.
Depends on: 147892
(In reply to comment #13)
> Serge, are you currently working on this? Maybe I could help a little.

I have stopped working on this for a while:
you are free/welcome to help as much as you want ;-)
Attached patch Patch (obsolete) — Splinter Review
This converts all <window class="dialog"...> to <dialog>. I also included some
minor cleanup like adding missing accesskeys etc that are IMHO not worth
separate bugs and reviews. I hope this is ok.

Sidenote: I also filed bug 245736 and bug 245728 to remove two unused <window
class="dialog"...>.
Sidenote 2: There are some <dialog>s that include dialogOverlay.xul in
mailnews/. This makes no sense, does it?
Attachment #150333 - Flags: review?(neil.parkwaycc.co.uk)
Depends on: 245728, 245736
Just reading the patch, you weren't consistent on your access keys - sometimes
you used uppercase, and sometimes you used the actual case of the letter.

I think the original intention was for the the new folder type radiobuttons to
be horizontal.

Also, there's a rename folder picker, which appears to be currently unused, so I
wouldn't remove the overlay, just comment it out, so that someone can fix it,
unless mscott wants to ditch that picker completely of course ;-)

I notice the "View" button in the compose security dialog looks lonely, I hope
to come up with a fix for that soon :-)
(In reply to comment #16)
> Just reading the patch, you weren't consistent on your access keys - sometimes
> you used uppercase, and sometimes you used the actual case of the letter.

Hmm, I don't see the problem here. I was trying to follow

  http://www.mozilla.org/projects/ui/accessibility/accesskey.html

(see especially paragraph "How is an accesskey added to a form control?" and the
first point of "How do I pick an accesskey letter?")

Have I misunderstood the rules given there?

> I think the original intention was for the the new folder type radiobuttons to
> be horizontal.

Ok, I changed this to orient="horizontal" class="indent".

> Also, there's a rename folder picker, which appears to be currently unused, so I
> wouldn't remove the overlay, just comment it out, so that someone can fix it,
> unless mscott wants to ditch that picker completely of course ;-)

Ok, I changed this so it is just commented out. IMO this picker can be removed.
The current UI for renaming folders is already to complex. Adding this picker
somewhere would make it even more complex. Instead inline renaming should be the
long-term goal in my opinion.

> I notice the "View" button in the compose security dialog looks lonely, I hope
> to come up with a fix for that soon :-)

That's cool. :-)
Ignore the access keys, I wasn't exactly awake at the time ;-)
Attached patch Patch V1.1 (obsolete) — Splinter Review
Updated patch. The only things changed are the two mentioned in comment 17.
Attachment #150333 - Attachment is obsolete: true
Attachment #150333 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150826 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 150826 [details] [diff] [review]
Patch V1.1

>+        ondialogcancel="return onCancelReplication();"
>+        onclose="return onCancelReplication();">
You don't need onclose in <dialog>s.

>+  <textbox tabindex="0" id="name" oninput="doEnabling();"/>
Please avoid tabindex.

>+        <radio id="some" selected="true" label="&download.label;"
>+               accesskey="&download.accesskey;" oncommand="setupDownloadUI(true);"/>
Nit: if you're going to wrap, do make sure that *all* of your lines are under
80 columns...

>+      var recipientsButton = document.documentElement.getButton("disclosure");
>+      recipientsButton.setAttribute("value", buttonlabels.getAttribute("recipientsLabel"));
>+      recipientsButton.removeAttribute("hidden");
Nit: to show this you would change the buttons attribute on your dialog
element.

>-function Help()
>-{
>-  return false;
>-}
Not sure that you should remove this; instead add an ondialoghelp for the
benefit of whomever should write the help.
Attachment #150826 - Flags: review?(neil.parkwaycc.co.uk) → review-
Comment on attachment 150826 [details] [diff] [review]
Patch V1.1

I forgot to mention that you left all the dialogs with the default buttonpack
of right although three of them used to have center packing.
(In reply to comment #21)
> (From update of attachment 150826 [details] [diff] [review])
> I forgot to mention that you left all the dialogs with the default buttonpack
> of right although three of them used to have center packing.

I omitted the buttonpack attribute deliberately. I don't see a reason to deviate
from the dialog XBL default of buttonpacking. Why should some dialogs have their
buttons centered and some right aligned? Are there any rules for this?

IMHO the buttonpack attribute is a source for inconsistencies. E.g. compare the
New Address Book Card dialog and the Address Book Card Properties dialog in
Seamonkey. Perhaps it should just be removed from everywhere. Toolkit seems to
ignore this attibute already (in TB on Linux the both Address Book dialogs just
mentioned don't have the inconsistent button placement. Also in TB all dialog
buttons are right aligned, even the OK button of simple alerts.).
Addressed all review comments.

For the record the result of a discussion with Neil on IRC: Dialogs that ask
the user a question should have the buttonpack="center" attribute. The
addressbook dialogs mentioned in comment 22 don't use it correctly.

So the "Download Headers" and the "Ask Send Format" dialogs now have
buttonpack="center".
Attachment #150826 - Attachment is obsolete: true
Attachment #151099 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #151099 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #151099 - Flags: superreview?(mscott)
No longer depends on: 245728
Comment on attachment 151099 [details] [diff] [review]
Patch V1.2 (checked in)

sorry for the delay
Attachment #151099 - Flags: superreview?(mscott) → superreview+
Comment on attachment 151099 [details] [diff] [review]
Patch V1.2 (checked in)

Patch checked in.

I leave this bug open for a follow-up patch that removes the inclusion of
dialogOverlay.* from files that don't use it.

For example:

http://lxr.mozilla.org/mozilla/source/mailnews/base/prefs/resources/content/Smt
pServerEdit.xul#4
Attachment #151099 - Attachment description: Patch V1.2 → Patch V1.2 (checked in)
Attached patch More cleanup (obsolete) — Splinter Review
This patch removes all references to dialogOverlay.* from files that don't use
it. 

I don't think certFetchingStatus.xul should be converted to <dialog> because it
didn't use anything from dialogOverlay.* except class="dialog" and doesn't have
the typical dialog buttons.

I also removed a <separator> in SmtpServerList.xul. I don't know how this was
still there, because I thought I removed it already in bug 223295.
Attachment #152606 - Flags: review?(neil.parkwaycc.co.uk)
I think you could make it a dialog... but you would have to adapt stopFetching
to be your close button, and you might want to wait for my dialog label patch
anyway to make it easier to specify the label.
Comment on attachment 152606 [details] [diff] [review]
More cleanup

Except certFetchingStatus.
Attachment #152606 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 152606 [details] [diff] [review]
More cleanup

Ok, I won't checkin the changes to certFetchingStatus.xul and convert it later
to a <dialog> instead. Asking for sr for the rest.

Neil: What is the bug number for your dialog label patch? Or haven't you filed
one yet?
Attachment #152606 - Flags: superreview?(mscott)
It's in bug 78274. And I only just noticed alecf's holiday message :-(
Attachment #152606 - Attachment is obsolete: true
Attachment #152606 - Flags: superreview?(mscott)
Comment on attachment 154129 [details] [diff] [review]
More cleanup V1.1 (checked in)

Neil, this is the same patch as before. I just included the conversion of
certFetchingStatus to <dialog> you suggested into this patch. So you only need
to review the changes in certFetchingStatus.*. Thanks! :-)
Attachment #154129 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #154129 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #154129 - Flags: superreview?(mscott)
Attachment #154129 - Flags: superreview?(mscott) → superreview+
Attachment #154129 - Attachment description: More cleanup V1.1 → More cleanup V1.1 (checked in)
There are still four references to dialogOverlay.* left in mailnews/:

1.
http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/resources/content/abSelectAddressesDialog.xul#42

This dialog still needs to be converted to <dialog>. When I tried it, it always
resulted in a blank window. So I need to figure out why this happens.

2.
http://lxr.mozilla.org/mozilla/source/mailnews/base/search/resources/content/CustomHeaders.xul#40

Needs to be converted to <dialog>. Needs some special care to not break the
current behaviour according to the keyboard navigation.

3.
http://lxr.mozilla.org/mozilla/source/mailnews/import/resources/content/importDialog.xul#41

Needs to be converted to <wizard> rather than to <dialog>. This is bug 101874. I
started working on this a few months ago and have a unfinished patch lying around. 

4.
http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/resources/content/abListOverlay.xul#53

This one I just overlooked. I will include the removal of this line in one of
the other patches I still need to do.
Product: Browser → Seamonkey
QA Contact: esther → search
QA Contact: search → message-display
Depends on: 1106540
You need to log in before you can comment on or make changes to this bug.