Closed Bug 415810 Opened 12 years ago Closed 12 years ago

Respect the user's settings of icons in menus

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: ventnor.bugzilla, Assigned: ventnor.bugzilla)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Currently we display icons in menus no matter what. GTK does have a setting as to whether to display icons on menuitems or not, and we should honour that since "light" WM's like Fluxbox don't have icons in their menus.
Attachment #301558 - Flags: superreview?(roc)
Attachment #301558 - Flags: review?(roc)
How about getting this metric via gtk2drawing.c? Then we don't have to create any new widgets.
Attached patch Patch 2 (obsolete) — Splinter Review
Assignee: nobody → ventnor.bugzilla
Attachment #301558 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #301764 - Flags: superreview?(roc)
Attachment #301764 - Flags: review?(roc)
Attachment #301558 - Flags: superreview?(roc)
Attachment #301558 - Flags: review?(roc)
Comment on attachment 301764 [details] [diff] [review]
Patch 2

+        aMetric = (moz_gtk_images_in_menus() ? 1 : 0);

Just return moz_gtk_images_in_menus() directly
Attachment #301764 - Flags: superreview?(roc)
Attachment #301764 - Flags: superreview+
Attachment #301764 - Flags: review?(roc)
Attachment #301764 - Flags: review+
Attached patch Patch 3 (obsolete) — Splinter Review
This patch will allow further integration with the user's interface preferences without adding much risk at all.
Attachment #301764 - Attachment is obsolete: true
Attachment #301769 - Flags: approval1.9?
Version: unspecified → Trunk
Comment on attachment 301769 [details] [diff] [review]
Patch 3

a1.9+=damons
Attachment #301769 - Flags: approval1.9? → approval1.9+
|visibility: hidden| means that all images will still be loaded and processed. Instead, we shouldn't even use the menuitem-iconic binding in that case.
Dao, I tried that but I think it still ended up using the iconic binding.

Either way, this method allows us to hide any and all icons on all menuitems and submenus, while still allowing proper margins.
(In reply to comment #7)
> Dao, I tried that but I think it still ended up using the iconic binding.

You think? :) Any chance to get that verified?
It's waste to load all icons for nothing.

> Either way, this method allows us to hide any and all icons on all menuitems
> and submenus, while still allowing proper margins.

The non-iconic binding should have proper margins, too. Otherwise that would be a bug.
I'll probably file a follow-up bug on that, but this method ensures no icons get displayed on any menuitem. I can't seem to find a way from within CSS to remove every use of the iconic binding, just the ones I had set in Gnomestripe.
menuitem {
  -moz-binding: url(chrome://global/content/bindings/menu.xml#menuitem) !important;
}
menu.menu-iconic {
  -moz-binding: url(chrome://global/content/bindings/menu.xml#menu) !important;
}
Also, if you put it in xul.css with the appropriate ifdef, it would work for non-default themes.
Should I not land this due to Dão's comments?
Land it now so users can get the benefit now, I'll file a followup bug for perf.
Sorry, but sayrer questions me on an almost daily basis about gtk/Linux perf issues, so if there's a known perf problem with this patch, I'd rather wait until there's a correct patch that fixes the problem and won't hurt perf. Don't want to be the bad guy here, but we don't need any more perf regressions.
Keywords: checkin-needed
(In reply to comment #13)
> Land it now so users can get the benefit now, I'll file a followup bug for
> perf.
> 

No...
Attached patch Patch 4 (obsolete) — Splinter Review
I dunno, this approach seems a little messy in xul.css, unless I misunderstood what everyone wanted...
Attachment #301769 - Attachment is obsolete: true
Attachment #302385 - Flags: superreview?(roc)
Attachment #302385 - Flags: review?(roc)
Attachment #302385 - Flags: review?(gavin.sharp)
(In reply to comment #14)
> Sorry, but sayrer questions me on an almost daily basis about gtk/Linux perf
> issues,

This isn't a performance regression so a followup bug would be fine here. In fact, the newest patch could be one (although very negligible), as it adds more complexity to xul.css
Comment on attachment 302385 [details] [diff] [review]
Patch 4

If you use something along the lines of comment 10 in xul.css (with more intelligent selectors, of course), you shouldn't need to patch gnomestripe at all, and no third-party theme would need to be updated.
e.g.:

> %ifdef MOZ_WIDGET_GTK2
> menuitem:not([type]):not(:-moz-system-metric(images-in-menus)) {
>   -moz-binding: url(chrome://global/content/bindings/menu.xml#menuitem) !important;
> }
> menu.menu-iconic:not(:-moz-system-metric(images-in-menus)) {
>   -moz-binding: url(chrome://global/content/bindings/menu.xml#menu) !important;
> }
> %endif

This is untested and not necessarily complete.
We should take patch 3. It's not going to hurt perf at all, in fact it will help perf a little bit for the small number of users it will affect. The performance "issue" is only that patch 3 might not help perf as much as it could ... but again, it's only going to affect a small minority of users so we shouldn't care much either way.
(In reply to comment #20)
> It's not going to hurt perf at all, in fact it will
> help perf a little bit for the small number of users it will affect.

The perf win, if existing, would be near to nothing compared to what could be won if you just don't load the icons, wouldn't it?

Also, I don't see why this should be fixed within gnomestripe. This is filed as a Core bug to start with. Should every single theme explicitly support this?
Actually, IIRC display:none doesn't load its content. I think I now know of a rule to use thos.
Hmm, turns out the images are also used for alignment due to their defined sizes, so that didn't work.
Attached patch Patch 5 (obsolete) — Splinter Review
The backend stuff has already gotten review from roc and it hasn't changed, so I'll just ask gavin to review the xul.css stuff.
Attachment #302385 - Attachment is obsolete: true
Attachment #302472 - Flags: review?(gavin.sharp)
Attachment #302385 - Flags: superreview?(roc)
Attachment #302385 - Flags: review?(roc)
Attachment #302385 - Flags: review?(gavin.sharp)
Attached patch Patch 6Splinter Review
I think this is the approach gavin wanted.
Attachment #302472 - Attachment is obsolete: true
Attachment #302697 - Flags: review?(gavin.sharp)
Attachment #302472 - Flags: review?(gavin.sharp)
Comment on attachment 302697 [details] [diff] [review]
Patch 6

Neil, can you take a look at this?
Attachment #302697 - Flags: review?(gavin.sharp) → review?(enndeakin)
Attachment #302697 - Flags: review?(enndeakin) → review+
Attachment #302697 - Flags: approval1.9?
Comment on attachment 302697 [details] [diff] [review]
Patch 6

>Index: toolkit/content/xul.css
What if I want to write a theme that has no intention of ever following OS settings? (OK, so Modern uses the OS font setting, but nothing else.)
(In reply to comment #27)
> (From update of attachment 302697 [details] [diff] [review])
> >Index: toolkit/content/xul.css
> What if I want to write a theme that has no intention of ever following OS
> settings? (OK, so Modern uses the OS font setting, but nothing else.)
> 

(In reply to comment #18)
> (From update of attachment 302385 [details] [diff] [review])
> If you use something along the lines of comment 10 in xul.css (with more
> intelligent selectors, of course), you shouldn't need to patch gnomestripe at
> all, and no third-party theme would need to be updated.
> 

Something has to give...
(In reply to comment #27)
> (From update of attachment 302697 [details] [diff] [review])
> >Index: toolkit/content/xul.css
> What if I want to write a theme that has no intention of ever following OS
> settings?

You override these rules.
Respecting the user's settings should be the default, not the other way around.
Never mind me. Modern also respects OS scrollbar styles.
Sorry for spamming, but what about menulists, e.g. in the helper app prefs?
Attachment #302697 - Flags: approval1.9? → approval1.9+
Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.80; previous revision: 1.79
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.54; previous revision: 1.53
done
Checking in widget/src/gtk2/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.37; previous revision: 1.36
done
Checking in widget/public/nsILookAndFeel.h;
/cvsroot/mozilla/widget/public/nsILookAndFeel.h,v  <--  nsILookAndFeel.h
new revision: 1.63; previous revision: 1.62
done
Checking in layout/style/nsCSSRuleProcessor.cpp;
/cvsroot/mozilla/layout/style/nsCSSRuleProcessor.cpp,v  <--  nsCSSRuleProcessor.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/content/xul.css;
/cvsroot/mozilla/toolkit/content/xul.css,v  <--  xul.css
new revision: 1.115; previous revision: 1.114
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Depends on: 564357
Depends on: 1302157
You need to log in before you can comment on or make changes to this bug.