185111 - Theme preview problems; patch

Status: VERIFIED INVALID

(Firefox :: Settings UI, defect)

Description

[blitzreiter]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5

I installed some other themes. Unfortunately not every theme is provided with a
preview pic. So the selector dialog creates some problems. 

Reproducible: Always

Steps to Reproduce:
1.Install a few themes and try it out
2.preferences dialogue, browse the themes
3.



Expected Results:  
Use a default preview graphic (no preview available) that the preferences dialog
does not switch from short list with preview to long list without. When unsing
you keyboard that may cause a loss of cursor.

Second help the theme editors to create a "preview picture". You should also
standardize the size of it.

Comment 1

[jon moses]

I'll agree with the "standardize the size" of the theme preview picture.  It's a
little frustrating to have most of the form controls change their size/position
when the theme preview image changes size.  Another possible solution would be
to limit the size of the display, and allow it so scroll.

Comment 2

[Alfred Kayser]

Not only the preview image size does create this problem,
also the size of the 'description' text, and if skinVersion
is incorrect, the extra message about 'outdated theme' also
causes shifting...

Note, that this problem is not in Mozilla, which uses the 
same preference panel. Why the differences?
The difference seems to be that Phoenix doesn't set
fixed sizes for the image and the description fields.

This is one of the areas of unneeded differences between
Mozilla and Phoenix causing themers extra pain and trouble!

Comment 3

[piers]

Attachment: Patch to stop elements moving about as much

Try to clean up the themes dialog a bit. Scales preview images to 380 x 85.
Theres still problems with long descriptions/authors.

(PS, this is my first patch so please be gentle :o)

Comment 4

[David P James]

Definitely a problem... just about anything can mess it up; image size, author
field with too many authors (I moved my name out of the author field in Pinball
just to avoid this) and description length.

Thank you for the patch Piers. Hopefully the developers will be able to use it
in some fashion.

-->Confirming as New
-->Added 'patch' to summary

Comment 5

[piers]

Attachment: Patch to stop elements moving about as much v2

Version 2...

Comment 6

[piers]

Attachment: Patch to stop elements moving about as much v3

Version 3.

Comment out the infoline at the top and make the dialog resizable.

Comment 7

[piers]

Attachment: Patch to stop elements moving about as much v3.1

Um, this is a patch, honest :o)

Comment 8

[David P James]

Sorry Piers... that still hasn't fixed it in Linux yet. But I'll get something
up here when I can be sure that I've got my + and - right.

Comment 9

[piers]

David, could you post screenshots to show what's wrong?

Also, bug 187188 affects this, see it's description for details.

Comment 10

[David P James]

Attachment: Theme Preferences Tab - missing text

Comment 11

[David P James]

Ok, I just lost my comment doing that... I was going to say that that screenshot
was based on a patch to pref-themes.xul posted by someone (Scratch - possibly
you?) on the Mozillazine forums and that it is similar to what I saw with v3.1
of the patch here (which I deleted and don't want to redo). It gives the right
impression though. The next screenshot is what it looks like with my patch which
has yet to be uploaded here.

Comment 12

[David P James]

Attachment: Screenshot with forthcoming (hopefully) patch

Comment 13

[piers]

David, re your second screenshot, there's not really enough space on Windows to
move the authors line down. I presume the install jar theme thing is still just
an extension.

Comment 14

[David P James]

Re comment #11

Sorry - that should have read Spark, not Scratch, as the creator of the
pref-themes.xul patch.

Comment 15

[piers]

Attachment: Screenshot on Win2K with patch v3.1

This still allows a fair bit of space for the author IMHO, but means the
description can be longer.

Comment 16

[David P James]

Attachment: Windows: theme prefs tab under [forthcoming] patch

Looks fine to me...

Comment 17

[piers]

Ok, you've made the theme list smaller, but i still kinda prefer my layout. If
other people think otherwise i'll just change my patch (i think it's better if
there's not two sets of patches).

Comment 18

[piers]

Attachment: Patch to stop elements moving about as much v4

Ok, change according to suggestions by David James (move the author line down
so that there can be longer authors).

Comment 19

[David P James]

Attachment: Appearance of patch v4 on Linux

This screenshot shows how patch v4 appears on Linux. As you can see, the
Uninstall button still gets text cut off. For this to work that max size needs
to be larger than 115 (and Pinball 0.6 is an average-sized theme title); I set
it to 180 but that might be overdoing it. I have it so that both the Header 
line and the Uninstall button can max out at 180, rather than 115 and 240 as is
the case in v4. If you think about it, the uninstall button *needs* more space
than the header as it contains everything that is in the header (eg the theme
title) *plus* the word 'Uninstall' as well as the requisite spacing of a
button. Together, the two cannot sum to more than ~360 without breaking things,
so we have space to give the button ~150-160 and the header ~200 or some other
like combination.

As an aside, if you diff patch v4 you'll find that you're missing a
<separator/> on or about line 84 of prefs-theme.xul. I don't know if that is
critical or not, but I left it in when I built prefs-theme.xul based on v4 for
testing to give the attached screenshot.

Otherwise, I am quite happy with v4.

Comment 20

[David P James]

Attachment: Patch to stop elements moving about as much v4.1

Piers,

I'd like you to check this patch out on Windows if you could. It sets the max
width of the Uninstall button to 160 (frankly, I would prefer it if the button
were just big enough for the word "Uninstall" and do away with including the
theme name altogether, but given that we have the name it doesn't look right to
be cutting it in two) and the max width of the title header to 195. I also
reinserted that <separator /> tag on or about line 84 on the grounds that it
didn't seem like a good thing to remove :)

Could you also check my diff (I think that's what it is called) to make sure
that it makes sense as this is my first one? I based it on yours so it should
be accurate.

I'll put a screenshot up shortly.

Comment 21

[David P James]

Attachment: Screenshot on Linux of theme prefs with patch v4.1

Here's a screenshot of the theme prefs tab with patch v4.1 on Linux.

Comment 22

[piers]

Attachment: Patch to stop elements moving about as much v5

Ok, this changes things to say "Uninstall Theme" for everything. The
<separator/> is removed so there is more space for the description (it means
that it is vertically aligned with the "Get New Extensions" link on the other
tab).

Also, makes sure the active theme is visible in the listbox
(ensureIndexIsVisible).

This should be the final patch now.

David, you shouldn't be making diffs by hand!!
http://www.gnu.org/software/diffutils/diffutils.html . Also, could you obsolete
your v4.1 patch because i don't have permissions. Thanks.

Comment 23

[David P James]

Attachment: Screenshot on Linux of theme prefs with patch v5

Comment 24

[David P James]

Comment on attachment 110435 [details] [diff] [review]
Patch to stop elements moving about as much v5

I like how the theme prefs tab appears with patch v5. The theme list stays the
same size, the preview space stays the same size, the Uninstall Theme button is
always the same size and in the same place, no conflicts between
medium-longish+ author names and the uninstall button.

Presumably it looks similar in Windows; I'll reboot and put up a screenshot
shortly and obsolete the relevant prior screenshots.

-->Set Flag to Review Status for Blake

Comment 25

[David P James]

Attachment: Screenshot on Windows 98SE of theme prefs with patch v5

Windows screenshot of the same thing; due to another bug I can't install any
other themes in Windows at the moment so this shot is rather bleak
unfortunately.

Last attachment (I promise :)

Comment 26

[Jason Kersey]

Comment on attachment 110435 [details] [diff] [review]
Patch to stop elements moving about as much v5

Correct me if I'm wrong, but I think most of these changes need to be in the
theme file, and not in the XUL.  Theme authors may want to change the way this
panel works in their theme.

Comment 27

[piers]

These changes prevent the dialog from screwing up (the theme preview becoming
too big for the dialog, the uninstall button from moving about and more). Why
would this be done theme by theme?

Comment 28

[piers]

Ok, max-width, max-height et al should be set in CSS (since they affect 
appearance, not meaning).

For some reason, the current CSS code to do this doesn't seem to work.

Comment 29

[David P James]

Jason: not to be rude, but have you actually looked at the theme preview dialog
presently with themes from different authors? Have you tried implementing the patch?

The Phoenix preview dialog moves around something ferocious which doesn't happen
with Mozilla's. Even if we agreed on a standard theme pic size and every theme
employed it we'd still have problems with differing lengths of descriptions.

If we're not going to accept patch v5 or something like it then this bug might
as well be marked WONTFIX. The problem is Phoenix's, not the theme's.

Comment 30

[blitzreiter]

I agree: Phoenix has to be more robust. Please fix the dialogue or let phoenix 
reject themes that do not confirm with standards yet to be defined. For 
instance each of them should have a preview picture. Each of them shall have a 
visible uninstall option. With some themes you have to resize the dialogue in 
order to see the uninstall button.

It would also be nice to have a theme builder's guide to Phoenix. I believe 
that "Preferences" is a problem to be fixed..

Comment 31

[Asa Dotzler [:asa]]

this bug is moot now that the new themes pref panel is in.

Comment 32

[David P James]

I would have said that the reorganization fixed the problem rather than
invalidated it, but, no matter, the problem no longer exists. If you haven't
already, check out the following MozillaZine topic for info on how to update
your theme for post-20030201 builds:

http://www.mozillazine.org/forums/viewtopic.php?t=5341

Comment 33

[noririty]

Mass-verifying INVAs.