Closed
Bug 63941
Opened 24 years ago
Closed 16 years ago
New Card layout shouldn't assume high screen resolution
Categories
(MailNews Core :: Address Book, defect, P2)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b1
People
(Reporter: caseyperkins, Assigned: friedrich.beckmann)
References
(Depends on 1 open bug)
Details
(Keywords: access, helpwanted, uiwanted, Whiteboard: nab-visual)
Attachments
(9 files, 10 obsolete files)
46.74 KB,
image/png
|
Details | |
26.42 KB,
image/png
|
Details | |
80.24 KB,
image/png
|
Details | |
14.31 KB,
image/png
|
clarkbw
:
ui-review+
|
Details |
13.22 KB,
image/png
|
clarkbw
:
ui-review+
|
Details |
12.71 KB,
image/png
|
clarkbw
:
ui-review+
|
Details |
11.42 KB,
image/png
|
clarkbw
:
ui-review+
|
Details |
44.74 KB,
patch
|
Details | Diff | Splinter Review | |
4.56 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
When clicking the New Card button in the Address Book, the dialog box that
appears is too tall for any resolution lower than 1024*768, not to mention being
poorly positioned on the screen.
I am running on a monitor that can't do better than 800*600 well. When I click
the New Card button, the box appears down and to the right every time. If I pull
the title bar up to the very top of the screen, I can barely see the OK and
Cancel buttons just peeking above the taskbar. For someone running a 640*480
resolution (God bless 'em), the OK and Cancel buttons would be impossible to
reach. For 800*600, they are just decidedly inconvenient.
It seems the better approach would be one or more of the following:
1) Make better use of the available space (there is a lot of wasted space
between the 3 main categories)
2) Either move some of the info to another tab (or new one), or provide a "More"
button that expands the box horizontally, or opens an additional dialog box.
3) Center the dialog box (horiz. and vert.) on the screen when it appears. This
part at least should be an easy hack.
Comment 1•24 years ago
|
||
Reporter, what build are you using? There was a regression, recently fixed, in
which the dialog in question was sized too large.
Reporter | ||
Comment 2•24 years ago
|
||
I am using the most recent build.
Comment 3•24 years ago
|
||
WORKSFORME
Platform: PC
OS: Linux 2.2.16
Mozilla Build: 2001010221
Reporter try with a new profile my bet is that will fix it.
Reporter | ||
Comment 4•24 years ago
|
||
I have a lot invested in the profile I've got. Besides, why should that fix it?
Aren't window designs, sizes and positioning specified in XUL files and C++
code? What does this have to do with a new profile?
Reporter | ||
Comment 5•24 years ago
|
||
I downloaded Mozilla onto another computer, created a new profile, opened up
Address Book and clicked New Card. Once again, the window sizing and positioning
were terrible for 800*600 and 640*480 resolutions. So creating a new profile
does not help, unfortunately.
Please take a look at the screenshot page I prepared in order to demonstrate
what I am talking about, and try it also on your own computer.
<a href="www.caseyperkins.com/newcard.html">www.caseyperkins.com/newcard.html
</a>
Thanks.
Comment 6•24 years ago
|
||
Marking NEW as per reporters comments.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 8•24 years ago
|
||
os/platform -> all, since the duplicate is reported on a Mac running System 9.
OS: Windows 98 → All
Hardware: PC → All
Reporter | ||
Comment 11•24 years ago
|
||
It seems like fixing would be a simple matter of x and y positioning and sizing.
I do some GUI programming (in Java) and that would be how I would do it there.
Can it be that different in C++/XUL?
As an example of centering the box in reference to the Mozilla window:
int height=450;
int width=300;
newCardBox.setSize(width,height);
int setLeft=(((mozWindow.getWidth()-width)/2)+mozWindow.getX());
int setRight=(((mozWindow.getHeight()-height)/2)+mozWindow.getY());
newCardBox.setLocation(setLeft,setRight);
newCardBox.show();
Yes, its Java pseudo-code, not C++, but the logic is good. Can't something
similar to this be implemented for the New Card box?
Comment 12•24 years ago
|
||
*** Bug 84990 has been marked as a duplicate of this bug. ***
Comment 13•23 years ago
|
||
*** Bug 94805 has been marked as a duplicate of this bug. ***
Comment 14•23 years ago
|
||
*** Bug 97041 has been marked as a duplicate of this bug. ***
Comment 15•23 years ago
|
||
Marking nsbeta1 since running at a resolution of 640x480 makes the new card
dialog unusable. It's so large that the bottom half of the dialog cannot be
seen, there is no way to traverse to the bottom to select the OK/Cancel buttons.
Resolution 600x800 is also too large but I can atleast move the dialog to access
the OK/Cancel buttons.
Keywords: nsbeta1
Updated•23 years ago
|
Whiteboard: nbaca-visual
Updated•23 years ago
|
Whiteboard: nbaca-visual → nb-visual
Updated•23 years ago
|
Whiteboard: nb-visual → nab-visual
Updated•23 years ago
|
Comment 17•23 years ago
|
||
Is this something hard to fix? I would think not. If not, please move this from
future to something sooner. There is simply no good reason for this to be so tall.
At 800 X 600 in Linux Mandrake KDE and in OS/2, the new card window is taller
than the space between the edge of the screen and the toolbar. In W2K at 800 X
600, it is only slightly less tall than that space, and opens way below 0,0,
leaving the OK & cancel buttons offscreen by default. In OS/2, it opens each
time with the titlebar way above the top of screen. See screenshot
http://bugzilla.mozilla.org/attachment.cgi?id=91249&action=view and bug 67913.
Simply right adjusting the currently left adjusted 'add to . . .' or the
'name,address,other' tabs would allow substantial vertical compaction without
losing [the excessive] whitespace between any entry fields.
The currently excessive whitespace means this could easily be made much shorter
than it currently is and accomodate minimum screen resolutions.
Comment 18•22 years ago
|
||
Attaching screenshot of new card in KDE, with KDE toolbar, at 800x600.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 20•20 years ago
|
||
As of "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040616"
I still see this bug (no ok and no cancel) on Win2k with a 3 window high
taskbar. This needs to be fixed and should be simple XUL I think.
Comment 21•20 years ago
|
||
As of "Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040616"
I still see this bug (no ok and no cancel) on Win2k 800x600 with a 3 window
high taskbar. This needs to be fixed and should be simple XUL I think.
Comment 22•20 years ago
|
||
*** Bug 165750 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Assignee: sspitzer → mail
Updated•19 years ago
|
Assignee: mail → nobody
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
Target Milestone: Future → ---
Comment 23•19 years ago
|
||
*** Bug 313058 has been marked as a duplicate of this bug. ***
Comment 24•19 years ago
|
||
*** Bug 293800 has been marked as a duplicate of this bug. ***
Comment 25•19 years ago
|
||
*** Bug 315223 has been marked as a duplicate of this bug. ***
Comment 26•19 years ago
|
||
*** Bug 317857 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Keywords: helpwanted
Comment 27•19 years ago
|
||
*** Bug 323507 has been marked as a duplicate of this bug. ***
Comment 28•19 years ago
|
||
This bug should be assigned a priority -- it affects the visually handicapped like me, and is a really ugly propblem on the "new card" pagelength problem reported as a "duplicate" to this report.
Comment 29•19 years ago
|
||
*** Bug 344234 has been marked as a duplicate of this bug. ***
Comment 30•18 years ago
|
||
*** Bug 349109 has been marked as a duplicate of this bug. ***
Comment 31•18 years ago
|
||
~633px is the minimum required height @ 96 DPI
Comment 32•18 years ago
|
||
The problem is actually worse now than when attachment 112016 [details] was affixed. DPI is now max(96dpi,system setting), which means unless a user has taken the trouble via user.js or about:config to force Gecko to use ~84 DPI or less, the card can't fit in the available vertical space, even with the taskbar hidden.
Comment 34•18 years ago
|
||
Ok, but is there a solution to this problem? Reassigning it to this bug number just moves it around.
Comment 35•18 years ago
|
||
There are a couple of solutions discussed in the Mozillazine forums:
http://forums.mozillazine.org/viewtopic.php?t=390988
http://forums.mozillazine.org/viewtopic.php?t=455604
These have to be manually added to a UserChrome.css file in each user profile though -- doesn't address the underlying problem.
Comment 37•17 years ago
|
||
bug 220443 has similar issue?
Comment 41•17 years ago
|
||
Its about time we put a proper solution together for this. Requesting appropriate flags. Bryan, any suggestions?
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?
Comment 42•17 years ago
|
||
Here's an unsolicited suggestion.
At the moment, Add Card contains three tabs (Contact - Address - Other). In these days of structured programming and OOD and all those other alphabet soups, how difficult would it be to distribute this over more tabs (and an obvious distribution would be Name - Internet - Phones - HomeAddress - WorkAddress - Other)? That would solve the problem at the expense of more tabs; which in my opinion is no price at all.
In case you are wondering why in 2008 I am using such a low-res screen, we bought in Singapore last week for the princely sum of the equivalent of about $US 300 an Asus Easy-Easy-Easy notebook with an 8.1" screen which has about 1024 X 600 resolution. Asus is predicting 2008-09 sales of tens of millions of these with Linux - and hence Thunderbird. Presumeably most of these people are going to want to add New Card.
Comment 43•17 years ago
|
||
It would be interesting to know how other addressbook UI (Evolution, Outlook, Outlook Express, etc.) addresses this problem (assuming they do)...
Keywords: uiwanted
Comment 44•17 years ago
|
||
URL doesn't work:
www.caseyperkins.com/newcard.html
Comment 45•17 years ago
|
||
Dan: I just tried this in Outlook Express. It organizes a "new contact" into five tabs. It has a variable size window with a minimum size that looks about 640x480, but no scroll bars, though. But that doesn't matter coz you finish it off with File -> Save and Close, which being in top LH corner you always see.
Comment 46•17 years ago
|
||
Ok, I'm collecting information on the dev wiki. I've collected screenshots of systems I have access to: Evolution and Outlook. I don't seem to have Outlook Express installed so I can't check that, if you have screenshots of other systems you could mail them to me and I'll post them as well.
Once we have a general understanding of how a number of other systems are tackling this problem we can start with some ascii art mockups, then move to more substance. Much of this is going to change or shape the fields available [2] and how they are presented.
In general I we should be able to fit into about an 800x600 window (with tabs), however it will take a number of changes to make that happen.
[1] http://wiki.mozilla.org/MailNews:Address_Book_New_Card
[2] http://wiki.mozilla.org/MailNews:Address_Book_Card_Fields
Comment 48•17 years ago
|
||
blocking‑thunderbird3+ given this can affect a lot more people with the rice of subnotebooks. Don't think this have to happen for 3.0a2.
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3+
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 51•16 years ago
|
||
I think we'll get to this in Tb3, but we probably wouldn't block a release for it.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Priority: -- → P2
Comment 52•16 years ago
|
||
What would be really nice is to just have it go more in the horizontal direction and eliminate all the tabs. It would be FAR, FAR easier to have it all in one place.
Assignee | ||
Comment 53•16 years ago
|
||
This is a patch for the abCardOverlay.xul file which rearranges the layout of the new card entry. It makes it wider and less tall in order to fit in 800x600 resolution.
Assignee | ||
Comment 54•16 years ago
|
||
This is a picture of the new arrangement of the boxes in the new card box with 800x600 resolution.
Assignee | ||
Updated•16 years ago
|
Attachment #338649 -
Flags: review?(bugzilla)
Updated•16 years ago
|
Assignee: nobody → friedrich.beckmann
Assignee | ||
Comment 55•16 years ago
|
||
Just to note: The patch is vs. Thunderbird version 2.0.0.16.
Assignee | ||
Comment 56•16 years ago
|
||
This patch is versus the latest 1.50 CVS version which is also the latest comm-central one. The previous patch is versus the version in 2.0.0.16.
The patch moves the "Phone Number" section to the right of "Name" and "Internet" in the first "Contact" tab. In the second tab, the two boxes are also placed side by side.
Attachment #338728 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #338728 -
Flags: review? → review?(bugzilla)
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 57•16 years ago
|
||
Are there any opinions on this layout: https://bugzilla.mozilla.org/attachment.cgi?id=338651?
Comment 58•16 years ago
|
||
Comment on attachment 338649 [details] [diff] [review]
Change the arrangement of the new card entry to allow 800x600 resolution (2.x branch)
I know you did this originally against the 2.x branch, and updated for trunk. However we won't accept changes like this on the 2.x branch because that is for security and stability only. So r- this patch just because of that. I'm looking at the trunk one separately.
Attachment #338649 -
Attachment description: Change the arrangement of the new card entry to allow 800x600 resolution → Change the arrangement of the new card entry to allow 800x600 resolution (2.x branch)
Attachment #338649 -
Flags: review?(bugzilla) → review-
Comment 59•16 years ago
|
||
Comment on attachment 338728 [details] [diff] [review]
This patch changes the order of the boxes for a new card. (vs 1.50)
We're now using Mercurial as our source control tool, so please can any future patches you supply use that please (see http://developer.mozilla.org/en/docs/comm-central for more info).
Although on Windows the layout fits within 800x600, on my mac, the dialog is 450x700 - obviously a bit too wide.
The fields on the notes page are also all to the right, which isn't good.
I also wonder if its possible to move the address book selector down alongside the ok/cancel buttons.
I think this may be a good start, but Bryan, our user experience expert, should also take a look at this, so I'll request ui-review from him.
Attachment #338728 -
Flags: ui-review?(clarkbw)
Comment 60•16 years ago
|
||
I need to see how the changes effect the other tabs as well. But overall the new arrangement leaves a lot of oddly stacked empty space. Perhaps we should be splitting things up differently?
Comment 61•16 years ago
|
||
(In reply to comment #60)
> I need to see how the changes effect the other tabs as well. But overall the
> new arrangement leaves a lot of oddly stacked empty space. Perhaps we should
> be splitting things up differently?
(In reply to comment #52)
> What would be really nice is to just have it go more in the horizontal
> direction and eliminate all the tabs. It would be FAR, FAR easier to have it
> all in one place.
Assignee | ||
Comment 62•16 years ago
|
||
Hi, thank you for your comments! In the meantime I returned my company laptop so it took a little while to respond. The new changes are versus the comm-central mercurial tree. I moved the home address and the birthday to a new tab. I will also add pictures of the 4 tabs for ui review. The size of the dialog box on windows is now 552x350. I guess the previous patch did not include the birthday changes... The patch changes the following files: abCardOverlay.xul, abCardOverlay.dtd and cardDialog.css. So it will requires changes in the language sets.
Attachment #338649 -
Attachment is obsolete: true
Attachment #338651 -
Attachment is obsolete: true
Attachment #338728 -
Attachment is obsolete: true
Attachment #342082 -
Flags: ui-review?(clarkbw)
Attachment #342082 -
Flags: review?
Attachment #338728 -
Flags: ui-review?(clarkbw)
Attachment #338728 -
Flags: review?(bugzilla)
Assignee | ||
Comment 63•16 years ago
|
||
This is a picture of the first tab for the new layout with the patched 552x350 version.
Attachment #342083 -
Flags: ui-review?(clarkbw)
Assignee | ||
Comment 64•16 years ago
|
||
Attachment #342084 -
Flags: ui-review?(clarkbw)
Assignee | ||
Comment 65•16 years ago
|
||
Assignee | ||
Comment 66•16 years ago
|
||
Attachment #342086 -
Flags: ui-review?(clarkbw)
Assignee | ||
Updated•16 years ago
|
Attachment #342085 -
Flags: ui-review?(clarkbw)
Comment 67•16 years ago
|
||
If you change this line in the birthday section:
<hbox class="AddressCardEditWidth">
to
<hbox class="AddressCardEditWidth" align="center">
The "or" should align itself a bit better.
Comment 68•16 years ago
|
||
What we need is FEWER tabs, not MORE tabs! Better still, eliminate ALL the tabs and put everything in one place. Why make it harder?
Assignee | ||
Comment 69•16 years ago
|
||
(In reply to comment #67)
> If you change this line in the birthday section:
> <hbox class="AddressCardEditWidth"> to
> <hbox class="AddressCardEditWidth" align="center">
> The "or" should align itself a bit better.
I changed it in my place for the moment. Good idea.
(In reply to comment #68)
> What we need is FEWER tabs, not MORE tabs! Better still, eliminate ALL the tabs
> and put everything in one place. Why make it harder?
The problem I want to solve is to make the card dialog fit to lower resolution displays. The original version is too high to fit to the Acer Aspire One which has 1024x600 resolution. You cannot even see the "Ok" and "Cancel" buttons so this is killing the whole address book on these devices. Putting everything in one place and eliminating all the tabs is probably not going in that direction. So I put the most important things in the first tab (Name, Email, Phone). The second tab is the private/home address and the third tab gives the business address. I split the home and business address to save some space. Do you think that removing the tabs will make the whole dialog smaller?
Assignee | ||
Updated•16 years ago
|
Attachment #342082 -
Flags: ui-review?(clarkbw) → ui-review?(bugmail)
Assignee | ||
Updated•16 years ago
|
Attachment #342082 -
Flags: ui-review?(bugmail) → ui-review?(clarkbw)
Assignee | ||
Comment 70•16 years ago
|
||
Today there was a short discussion on IRC between standard8, asuth and me regarding the address card edit. There is the idea that the address entries could also be edited "inline" in the "preview pane". At the moment the display of the preview pane can be disabled. Result of the the discussion is that the "inline-editing" is independent of the card dialog editing. It could be done regardless of this patch.
Comment 71•16 years ago
|
||
Comment on attachment 342083 [details]
Picture of the 1st tab for the 552x350 layout patch
I don't really like how the "Allow remote images" lines up here. But it's not too big a deal and I'm hopeful it could be removed (bug 457296) Otherwise things look fine.
Attachment #342083 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 72•16 years ago
|
||
Comment on attachment 342084 [details]
Picture of the 2nd tab for th 552x350 layout patch 342082
The City text box seems larger than it needs to be, however I don't know how you'd arrange the extra space. It's better to use it up like this than to have the empty block in the middle.
Attachment #342084 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 73•16 years ago
|
||
Comment on attachment 342085 [details]
Picture of the 3rd tab for the 552x350 layout patch 342082
Looks good overall
Attachment #342085 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 74•16 years ago
|
||
Comment on attachment 342086 [details]
Picture of the 4th tab for the 552x350 layout patch 342082
Not a pretty tab, but nothing to do with this bug. Looks good overall.
Attachment #342086 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 75•16 years ago
|
||
Comment on attachment 342082 [details] [diff] [review]
New Address Card Layout patch with 4 Tabs
I already gave the plus for all the screenshots, as usual I expect the other reviewer to look at the code itself.
Attachment #342082 -
Flags: ui-review?(clarkbw) → ui-review+
Assignee | ||
Comment 76•16 years ago
|
||
(In reply to comment #75)
> (From update of attachment 342082 [details] [diff] [review])
> I already gave the plus for all the screenshots, as usual I expect the other
> reviewer to look at the code itself.
thank you for the review!
(In reply to comment #71)
> (From update of attachment 342083 [details])
> I don't really like how the "Allow remote images" lines up here. But it's not
> too big a deal and I'm hopeful it could be removed (bug 457296) Otherwise
> things look fine.
I have it here in this tab because this is the "email" section. I do not know a better solution.
(In reply to comment #72)
> (From update of attachment 342084 [details])
> The City text box seems larger than it needs to be, however I don't know how
> you'd arrange the extra space. It's better to use it up like this than to have
> the empty block in the middle.
I was more concerned about the smaller department and title box which I put in one line. As you mention in your comment, the width is just there to make the city box that wide.
Again, thanks for the review!
Friedrich
Comment 77•16 years ago
|
||
Comment on attachment 342082 [details] [diff] [review]
New Address Card Layout patch with 4 Tabs
I'll do a more detailed review later, but my first comment is that abCardOverlay.xul is shared between SeaMonkey and Thunderbird.
So would you mind porting your changes to abCardOverlay.dtd and cardDialog.css to the files in the mail/ directory? (i.e. Thunderbirds).
Attachment #342082 -
Flags: review? → review?(bugzilla)
Assignee | ||
Comment 78•16 years ago
|
||
This is an updated patch which modifies
mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
mail/themes/pinstripe/mail/addrbook/cardDialog.css
mail/themes/qute/mail/addrbook/cardDialog.css
mailnews/addrbook/resources/content/abCardOverlay.xul
suite/locales/en-US/chrome/mailnews/addressbook/abCardOverlay.dtd
suite/themes/classic/messenger/addressbook/cardDialog.css
suite/themes/modern/messenger/addressbook/cardDialog.css
in order to not break the seamonkey code and the different themes. However, this is done in blind flight as I do not have a build environment. I only changed the code based on a nightly snapshot and copied it back to a mercurial source tree.
I also added the small change regarding the "or" positioning in the birthday.
Attachment #342082 -
Attachment is obsolete: true
Attachment #342082 -
Flags: review?(bugzilla)
Assignee | ||
Updated•16 years ago
|
Attachment #344934 -
Flags: review?(bugzilla)
Updated•16 years ago
|
Attachment #344934 -
Attachment is patch: true
Comment 79•16 years ago
|
||
Comment on attachment 344934 [details] [diff] [review]
New Card Layout - Patch now with seamonkey and thunderbird plus themes
>diff -r 785c708554f7 mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>--- a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd Mon Oct 27 14:33:43 2008 +0100
>+++ b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd Mon Oct 27 17:33:41 2008 +0100
>@@ -17,18 +17,18 @@
> The Initial Developer of the Original Code is
> Netscape Communications Corporation.
> Portions created by the Initial Developer are Copyright (C) 1998-1999
> the Initial Developer. All Rights Reserved.
>
> Contributor(s):
>
> Alternatively, the contents of this file may be used under the terms of
>- either the GNU General Public License Version 2 or later (the "GPL"), or
>- the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ either of the GNU General Public License Version 2 or later (the "GPL"),
>+ or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
Please don't change this. The license has to be a set format and wording as specified here: http://www.mozilla.org/MPL/boilerplate-1.1/
>@@ -75,49 +75,50 @@
> <!ENTITY PreferMailFormat.label "Prefers to receive messages formatted as:">
> <!ENTITY PreferMailFormat.accesskey "r">
> <!ENTITY PlainText.label "Plain Text">
> <!ENTITY HTML.label "HTML">
> <!ENTITY Unknown.label "Unknown">
> <!ENTITY ScreenName.label "Screen Name:">
> <!ENTITY ScreenName.accesskey "S">
>
>-<!ENTITY allowRemoteContent.label "Allow remote images in HTML mail.">
>+<!ENTITY allowRemoteContent.label "Allow remote images.">
> <!ENTITY allowRemoteContent.accesskey "w">
>+<!ENTITY allowRemoteContent.tooltip "In HTML messages it is possible to embed images from remote sources. Opening such a message will open a connection to this external source. This allows to track the reading of the message. This check box will allow such external embedded images in HTML messages.">
Slightly better wording:
"In HTML messages it is possible to embed images from remote sources. Opening such a message will open a connection to this external source. This may allow tracking of the message being read. Checking this box will allow such external embedded images in HTML messages from this contact."
> <!ENTITY Custom4.label "Custom 4:">
> <!ENTITY Custom4.accesskey "4">
>-<!ENTITY Notes.box "Notes">
>-<!ENTITY Notes.accesskey "N">
>+
No need for a blank line here, as long as there's just one new line after the last > we're fine.
> <tabpanels id="abTabPanels" flex="1">
>
> <!-- ** Name Tab ** -->
>- <vbox index="name" flex="1">
>- <groupbox flex="1">
>- <caption label="&Name.box;"/>
>+ <tabpanel>
>+ <vbox>
>+ <hbox index="name" flex="1">
>+ <vbox>
> <vbox>
If you look at the existing code, <tabpanel> isn't necessary.
You also have an extra <vbox> in here (after the <hbox>) that you don't need.
Unfortunately, that means you still have an extra box element, so you need to re-indent the lines it encloses by two spaces.
Once you have done this, please attach the full patch and a diff -w patch.
>+ <vbox>
>+ <hbox>
>+ <spacer flex="1"/>
>+ <label control="WorkPhone" value="&WorkPhone.label;" accesskey="&WorkPhone.accesskey;" class="CardEditLabel"/>
As you're now touching these lines, please can you wrap accesskey onto the next line and align it with the start of control. You may need to wrap the class attribute.
ditto with other similar lines that you touch please.
> <menulist id="PreferMailFormatPopup">
> <menupopup>
> <!-- 0,1,2 come from nsIAbPreferMailFormat in nsIAbCard.idl -->
> <menuitem value="0" label="&Unknown.label;"/>
> <menuitem value="1" label="&PlainText.label;"/>
> <menuitem value="2" label="&HTML.label;"/>
> </menupopup>
> </menulist>
>+
>+ <checkbox id="allowRemoteContent" label="&allowRemoteContent.label;"
>+ accesskey="&allowRemoteContent.accesskey;"
>+ tooltiptext="&allowRemoteContent.tooltip;"/>
> </hbox>
This should be indented inline with the </menulist> above.
>+ <hbox class="AddressCardEditWidth">
>+ <textbox id="JobTitle" flex="1"/>
> <label control="Department" value="&Department.label;"
> accesskey="&Department.accesskey;" class="CardEditLabel"/>
>- <hbox class="CardEditWidth">
>- <textbox id="Department" flex="1"/>
>+ <textbox id="Department" flex="1"/>
> </hbox>
Again, need to fix the indentation.
Mostly this is looking fine, I've verified it all seems to work on SM (both themes) & TB on Mac, I'll check it on Linux once you update the patch.
Attachment #344934 -
Flags: review?(bugzilla) → review-
Assignee | ||
Comment 80•16 years ago
|
||
This patch includes the comments from standard8. I changed the indention and removed useless tabs and boxes. There was no difference in the patch size between -U8 and -U8 -w, so I only included the -U8 diff. The files
mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
suite/locales/en-US/chrome/mailnews/addressbook/abCardOverlay.dtd
are identical. However there is a (small) difference in the license text in the original code. So I had to choose one to make the files really identical. I took the one from the template that standard8 mentionend. Thanks for the review!
Attachment #344934 -
Attachment is obsolete: true
Attachment #345282 -
Flags: review?(bugzilla)
Assignee | ||
Comment 81•16 years ago
|
||
(In reply to comment #79)
> >- either the GNU General Public License Version 2 or later (the "GPL"), or
> >- the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> >+ either of the GNU General Public License Version 2 or later (the "GPL"),
> >+ or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>
> Please don't change this. The license has to be a set format and wording as
> specified here: http://www.mozilla.org/MPL/boilerplate-1.1/
I reversed this. Now it complies to the template.
>
> Slightly better wording:
>
> "In HTML messages it is possible to embed images from remote sources. Opening
> such a message will open a connection to this external source. This may allow
> tracking of the message being read. Checking this box will allow such external
> embedded images in HTML messages from this contact."
I took your wording.
> >-<!ENTITY Notes.accesskey "N">
> >+
>
> No need for a blank line here, as long as there's just one new line after the
> last > we're fine.
Done.
> If you look at the existing code, <tabpanel> isn't necessary.
>
> You also have an extra <vbox> in here (after the <hbox>) that you don't need.
>
> Unfortunately, that means you still have an extra box element, so you need to
> re-indent the lines it encloses by two spaces.
>
> Once you have done this, please attach the full patch and a diff -w patch.
I removed the <tabpanel> and the useless <vboxes> and <hboxes>. The diff -U8 -w showed no difference to the diff -U8, so I only added the full patch.
>
> As you're now touching these lines, please can you wrap accesskey onto the next
> line and align it with the start of control. You may need to wrap the class
> attribute.
I did the wraps to have less line length overall.
>
> Mostly this is looking fine, I've verified it all seems to work on SM (both
> themes) & TB on Mac, I'll check it on Linux once you update the patch.
Thanks for review!
Comment 82•16 years ago
|
||
(In reply to comment #81)
> > Once you have done this, please attach the full patch and a diff -w patch.
> I removed the <tabpanel> and the useless <vboxes> and <hboxes>. The diff -U8 -w
> showed no difference to the diff -U8, so I only added the full patch.
Unfortunately diff -w doesn't always work in Mercurial, see https://developer.mozilla.org/en/Mercurial_FAQ#How_do_I.c2.a0get_a_diff_-w.3f for more info.
I'm just reviewing the patch though, so don't worry about adding a diff -w straight away.
Updated•16 years ago
|
Attachment #345282 -
Flags: review?(bugzilla) → review+
Comment 83•16 years ago
|
||
Comment on attachment 345282 [details] [diff] [review]
New Card layout - Updated patch including the comments from Standard8
> <tabpanels id="abTabPanels" flex="1">
>+ <!-- ** Name Tab ** -->
>+ <!-- The following vbox contains two hboxes top: Name/Email/Phonenumber bottom: Email prefs. -->
>+ <vbox>
>+ <!-- This hbox contains two vboxes left: Name/Email, right: Phonenumbers -->
>+ <hbox index="name" flex="1">
I think the index/flex attributes should be on the <vbox> element above.
I think the business vbox also needs an index.
Otherwise r=me with those fixed. You'll need an sr for this patch as well.
Assignee | ||
Comment 84•16 years ago
|
||
I removed the index attributes as they are useless. I added ids to the for tabs. They may be useful for future overlays.
Attachment #345282 -
Attachment is obsolete: true
Attachment #346231 -
Flags: superreview?
Attachment #346231 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #346231 -
Flags: superreview? → superreview?(neil)
Assignee | ||
Updated•16 years ago
|
Attachment #346231 -
Flags: review? → review+
Comment 85•16 years ago
|
||
Comment on attachment 346231 [details] [diff] [review]
Updated patch - removed the index attributes in abCardOverlay.xul
git-apply complained of 32 whitespace errors, of which at least 25 are new.
If you can't find them try /msg firebot review 346231 (note: it's designed to review C++ so some of its other quibbles aren't always appropriate.)
I thought the display looked a little crowded; the current version at least has those groupboxes that break up the dialog into manageable pieces.
>+<!ENTITY allowRemoteContent.tooltip "In HTML messages it is possible to embed images from remote sources. Opening
>+such a message will open a connection to this external source. This may allow
>+tracking of the message being read. Checking this box will allow such external
>+embedded images in HTML messages from this contact." >
I don't know how common wrapping long entities is, but you've wrapped the text in the entity rather than the text in the file, which looks odd. Also speaking of whitespace errors you can lose the space before the >
> .CardEditWidth {
>- width: 25em;
>+ width: 21em;
Is it possible to change these widths to the equivalent in ch?
(This affects themes/platforms that use a bold font)
> .alignBoxWithFieldset {
> margin-left: 6px;
> margin-right: 5px;
> }
You don't use this any more. (Both comments apply for all themes of course.)
>+ <hbox id="nickNameContainer">
>+ <spacer flex="1"/>
>+ <label control="NickName" value="&NickName.label;"
>+ accesskey="&NickName.accesskey;" class="CardEditLabel"/>
>+ <hbox class="CardEditWidth">
>+ <textbox id="NickName" flex="1"/>
> </hbox>
> </hbox>
Just picking on this one because it was easy to extract on the diff, but this applies to most of the fields in this window.
Rather than setting the top margin in the CardEditLabel class, I think the outer hbox needs an align="center" so the labels line up properly with the textboxes.
If you wanted to, you could get rid of the <spacer> by using <label flex="1"> and adding text-align: right; to the CardEditLabel class.
>- <caption>
>- <label control="Notes" value="&Notes.box;" accesskey="&Notes.accesskey;"/>
>- </caption>
This caption was important as it labelled the Notes control...
Assignee | ||
Comment 86•16 years ago
|
||
This patch includes changes based on the comments from Neil.
* I removed the cardEditLabel class altogether. The alignment of the labels is done via the align="center" attributes.
* Changed the css unit from em to ch
* Added the notes label
Thanks for the review!
Attachment #346231 -
Attachment is obsolete: true
Attachment #346254 -
Flags: superreview?(neil)
Attachment #346231 -
Flags: superreview?(neil)
Assignee | ||
Comment 87•16 years ago
|
||
This patch includes changes based on the comments from Neil.
* I removed the cardEditLabel class altogether. The alignment of the labels is
done via the align="center" attributes.
* Changed the css unit from em to ch
* Added the notes label
* Reduced the whitespace and longline style bugs
Thanks for the review!
Attachment #346254 -
Attachment is obsolete: true
Attachment #346259 -
Flags: superreview?(neil)
Attachment #346254 -
Flags: superreview?(neil)
Assignee | ||
Comment 88•16 years ago
|
||
(In reply to comment #85)
> (From update of attachment 346231 [details] [diff] [review])
> git-apply complained of 32 whitespace errors, of which at least 25 are new.
> If you can't find them try /msg firebot review 346231 (note: it's designed to
> review C++ so some of its other quibbles aren't always appropriate.)
Thanks for the hint. I removed the whitespace and longline errors but there are 4 left in the current patch. I have changed it locally here for the moment.
>
> I thought the display looked a little crowded; the current version at least has
> those groupboxes that break up the dialog into manageable pieces.
I removed the groupboxes on purpose because they consume quite some space. Because I address is now split in home and business, that one is obsolete. For the phone numbers and Emails on the first tab, I compared it to Outlook where they also have it like this. So in the end I removed them only because of the space.
> >+<!ENTITY allowRemoteContent.tooltip "In HTML messages it is possible to embed images from remote sources. Opening
> >+such a message will open a connection to this external source. This may allow
> >+tracking of the message being read. Checking this box will allow such external
> >+embedded images in HTML messages from this contact." >
> I don't know how common wrapping long entities is, but you've wrapped the text
> in the entity rather than the text in the file, which looks odd. Also speaking
> of whitespace errors you can lose the space before the >
I split it a little differently to have no line longer than 80 but I left it essentially.
>
> > .CardEditWidth {
> >- width: 25em;
> >+ width: 21em;
> Is it possible to change these widths to the equivalent in ch?
> (This affects themes/platforms that use a bold font)
Done.
>
> > .alignBoxWithFieldset {
> > margin-left: 6px;
> > margin-right: 5px;
> > }
> You don't use this any more. (Both comments apply for all themes of course.)
Removed it.
>
> >+ <hbox id="nickNameContainer">
> >+ <spacer flex="1"/>
> >+ <label control="NickName" value="&NickName.label;"
> >+ accesskey="&NickName.accesskey;" class="CardEditLabel"/>
> >+ <hbox class="CardEditWidth">
> >+ <textbox id="NickName" flex="1"/>
> > </hbox>
> > </hbox>
> Just picking on this one because it was easy to extract on the diff, but this
> applies to most of the fields in this window.
> Rather than setting the top margin in the CardEditLabel class, I think the
> outer hbox needs an align="center" so the labels line up properly with the
> textboxes.
> If you wanted to, you could get rid of the <spacer> by using <label flex="1">
> and adding text-align: right; to the CardEditLabel class.
I removed the CardEditLabel class and did the alignment with the attribute.
>
> >- <caption>
> >- <label control="Notes" value="&Notes.box;" accesskey="&Notes.accesskey;"/>
> >- </caption>
> This caption was important as it labelled the Notes control...
I added the label again.
Thanks for the review!
Assignee | ||
Comment 89•16 years ago
|
||
Modified the access keys and removed the remaining 4 whitespace errors.
Should be possible to navigate throught the dialog with keys.
Attachment #346259 -
Attachment is obsolete: true
Attachment #346259 -
Flags: superreview?(neil)
Assignee | ||
Updated•16 years ago
|
Attachment #346269 -
Flags: superreview?(neil)
Updated•16 years ago
|
Attachment #346269 -
Flags: superreview?(neil) → superreview+
Comment 90•16 years ago
|
||
Comment on attachment 346269 [details] [diff] [review]
Updated patch - modified the accesskeys
> <!ENTITY WorkZipCode.label "ZIP/Postal Code:">
>-<!ENTITY WorkZipCode.accesskey "P">
>+<!ENTITY WorkZipCode.accesskey "z">
Nit: Prefer to match the case of the accesskey to the label's character.
Assignee | ||
Comment 91•16 years ago
|
||
Just changed some access keys to capital letters.
Attachment #346269 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 92•16 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/4ae62e9a43c3
Thanks for all your work on this Fritz.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1
Assignee | ||
Comment 93•16 years ago
|
||
(In reply to comment #92)
> Checked in: http://hg.mozilla.org/comm-central/rev/4ae62e9a43c3
>
> Thanks for all your work on this Fritz.
Thank you, Clark, Neil and the others for your reviews and help!
Nice environment here.
Comment 94•16 years ago
|
||
<!ENTITY Birthday.label "Birthday:">
<!ENTITY Birthday.accesskey "B">
<!ENTITY Year.emptytext "Year">
<!ENTITY Or.value "or">
<!ENTITY Age.emptytext "Age">
I suggest to move these to the "Private" section to reflect the situation in UI.
Assignee | ||
Comment 95•16 years ago
|
||
This is a small patch to rearrange the entities in the abCardOverlay.dtd file. The birthday section is moved from the "Other Tab" to the "Private Tab" entries to be aligned with the display in the user interface address card dialog. This patch does not change behaviour.
Assignee | ||
Updated•16 years ago
|
Attachment #346645 -
Flags: review?(bugzilla)
Comment 96•16 years ago
|
||
Comment on attachment 346645 [details] [diff] [review]
Move birthday section in abCardOverlay.dtd as proposed in Comment #94
I'm not sure its absolutely vital that we do this. However, seeing as the patch is here and they are reasonably well organised in the file, we can take it.
Attachment #346645 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 97•16 years ago
|
||
(In reply to comment #96)
> (From update of attachment 346645 [details] [diff] [review])
> I'm not sure its absolutely vital that we do this. However, seeing as the patch
> is here and they are reasonably well organised in the file, we can take it.
The changes has been checked in: http://hg.mozilla.org/comm-central/rev/eb6e30c22d74
Thanks! Friedrich
Comment 100•16 years ago
|
||
The bug is still in:
thunderbird-2.0.0.19.en-US.win32.installer.exe
I guess the nightly's aren't.
The bugfix is in the 3.x series which isn't compatible with lightning.
Does anyone know when the bugfix will make into a nightly?
BTW, Thanks for fixing the bug : )
Comment 101•16 years ago
|
||
(In reply to comment #100)
> The bug is still in:
> thunderbird-2.0.0.19.en-US.win32.installer.exe
Patches like this will never make it into released major versions because the updates are for security and stability updates.
> The bugfix is in the 3.x series which isn't compatible with lightning.
> Does anyone know when the bugfix will make into a nightly?
If you mean a 2.x nightly then the answer is never. If you mean a 3.x nightly then it is already there and there are lightning versions available for the 3.x nightlies.
Comment 105•16 years ago
|
||
All the discussions regarding what fields go where - surely making the Edit Card scrollable is the best answer!!
See Also: → https://launchpad.net/bugs/115484
You need to log in
before you can comment on or make changes to this bug.
Description
•