File filters can make file selection dialog too wide for screen

Bug #219385 reported by mroth on 2008-04-18
218
This bug affects 26 people
Affects Status Importance Assigned to Milestone
GTK+
Fix Released
Low
One Hundred Papercuts
Low
Unassigned
Ubuntu
Undecided
Unassigned
Declined for Maverick by Sebastien Bacher
gtk+2.0 (CentOS)
New
Undecided
Unassigned
gtk+2.0 (Ubuntu)
Low
Unassigned
Declined for Maverick by Sebastien Bacher

Bug Description

Binary package hint: gnome-desktop-environment

This bug occurs in Firefox but is likely related to the generic Gnome file-selection-dialog rather than application specific.

In this case our website is calling OS file dialog via Firefox and JS/Flash, passing in a filter with a description (eg. "Photos and Videos"), and file extension-matching pattern (eg. "*.mov;*.mpg;*.avi;*.jpg")

Windows dialogs tend to truncate this list, and expand only the drop-down when selected, open and active. Ubuntu/Gnome appears to make the dialog width match the width of the filter drop-down, which is quite long due to the number of file types and mixed-case scenarios (eg. *.jpg;*.JPG)

Due to the amount of file formats we're filtering based on, the dialog box can easily grow wider than the user's display permits.

See screenshot for illustration:
http://farm4.static.flickr.com/3215/2403694395_ca70740726.jpg

Expected behavior would be to truncate the file filter list display past a certain point to prevent dialog box from growing too large.

Our testing confirms this behavior in both Ubuntu 7.10 and 8.04RC.

This bug prevents Ubuntu users from being able to use the advanced upload tool on Flickr.com from Firefox.

Changed in meta-gnome2:
status: New → Invalid
Alexander Sack (asac) wrote :

reassigning to gtk+ for now as ffox uses gtk dialog nowadays.

Pedro Villavicencio (pedro) wrote :

thanks for your report, that's known upstream you can track it here: http://bugzilla.gnome.org/show_bug.cgi?id=527499

Changed in gtk+2.0:
assignee: nobody → desktop-bugs
importance: Undecided → Low
status: New → Triaged
Changed in gtk:
status: Unknown → New
Changed in gtk:
status: New → Confirmed
Changed in hundredpapercuts:
milestone: none → round-3
status: New → Confirmed
Rich Jones (richwjones) wrote :

I've noticed this trying to upload to Flickr, something which a lot of users are likely to do, so this is a very relevant bug.

Jack (jackhynes) wrote :

Such an annoying bug, the only 'paper cut' I was absolutely sure of.

The non-parenthesized text in the file type filter combo box is going to be descriptive enough in most cases, and users will practically always try to pick the target file before checking the file type filter. Drawing inspiration from the Open dialog in GIMP, the attached mockup shows the file chooser in the Flickr scenario with file extensions omitted. The extensions should instead be placed in a tooltip (the tooltip should line-wrap to avoid the same problem of being too wide for the screen).

Changed in hundredpapercuts:
assignee: nobody → Canonical Desktop Team (canonical-desktop-team)
affects: meta-gnome2 (Ubuntu) → ubuntu
Martin Pitt (pitti) wrote :

The bug was forwarded upstream, and we don't have prominent applications which suffer from this, so we'll leave the fix to upstream.

Changed in gtk+2.0 (Ubuntu):
assignee: Ubuntu Desktop Bugs (desktop-bugs) → nobody
assignee: nobody → Ubuntu Desktop Bugs (desktop-bugs)
tags: added: ct-rev

Really? It seems that the flickr file upload experience is pretty prominent.

Rick Spencer [2009-08-03 15:53 -0000]:
> Really? It seems that the flickr file upload experience is pretty
> prominent.

I have never ever seen it, but I never used flickr either. If a common
use case exposes this, I'm fine if our team works on it. It just seems
like a very corner case to me.

Martin Brook (martin.brook) wrote :

The Flickr example is not the only case, although it is a prime example of the problem. I've encountered this several times in many different situations. It is quite confusing for a new user as it can appear as if the open button is simply missing.

Also, users of netbooks will tend to see this more often due to their smaller screens.

As Martin Brook said, it actually happens on netbooks. And not only for the dialog width but the height as well, as you can see in the snapshot. So, I suggest to consider both aspects.

I use an HP Mini 110 (1024×576)

Changed in hundredpapercuts:
milestone: round-3 → r2
Patrick Dickey (pdickeybeta) wrote :

Would the fix be as simple as having just the type ("Picture or Image File" or "Video File") in the dialog, and having a tooltip pop up that shows the various extensions available in that format? Or would that create a different paper cut (in that people may be used to seeing things a certain way--especially former Windows users)?

Have a great day:)
Patrick.

Vish (vish) on 2010-05-22
Changed in hundredpapercuts:
importance: Undecided → Low
status: Confirmed → Triaged
leonidas (l-wallner) wrote :

I just wanted to report that this bug also appears on my computer when i am using a web-fax-service in firefox. This website lets me upload lots of different file types to use as a fax, thus also the file choose dialog gets too long...
And this is definitely not a problem only happening in netbooks or systems with low display resolution, if there are enough file types in the filter list (as in the case of the fax-service), then it will also happen on "normal" desktops.

Changed in gtk:
status: Confirmed → Fix Released
David Tombs (dgtombs) wrote :

Fixed upstream? Holy cow, finally! Please get this in Maverick!

Vish (vish) wrote :

This has now been fixed upstream!

Changed in gtk+2.0 (Ubuntu):
assignee: Ubuntu Desktop Bugs (desktop-bugs) → nobody
status: Triaged → Fix Committed
Changed in hundredpapercuts:
status: Triaged → Fix Committed
Changed in gtk:
importance: Unknown → Low
Arek Olek (arekolek) wrote :

It didn't make it to Maverick did it? I've just updated and yet it's still there :/

3 comments hidden view all 156 comments

Created attachment 505773
Very WIP patch

GTK+ 3.0 and GNOME 3 are approaching and we should get Firefox ready for them.

Attaching WIP patch which I've been working on with Martin Stransky. Patch also contain changes from bug #611953 (which are going to be removed in final version).

So far with this patch we are able to compile mozilla-central with GTK3, but there is still much work to do (fix exposing issues and couple of crashes).

*** Bug 610234 has been marked as a duplicate of this bug. ***

Created attachment 513089
wip v2

updated version, against trunk, builds and runs. A gtk widget implementation is not finished yet, background is not rendered properly, sometimes crashes.

Created attachment 513457
wip v3

fixed background rendering and style borders

This looks like great work.

We need to think about how to support gtk2 for a while as well as gtk3. widget/src/gtk2 could be duplicated to widget/src/gtk3. Some of the rest of the code still works with gtk2 I guess.

Most of the changes are in widget/src/gtk2/gtk2drawing.c and widget/src/gtk2/nsWindow.cpp, other files may need some clean up only.

Created attachment 515080
wip v4

We focused on wrongly drawn widgets:
- buttons/checkboxs/ratio buttons
- scrollbars
- treeview
- tabs
- and few crashes
We still have issues with menu and tooltips and random crashes which Martin is investigating. Also fonts are pink in specific desktop configuration.

Created attachment 518712
WIP v5

Another WIP:
- Fixed wrong colors delived from GtkStyle
- Tabs better drawn
- Scrollbars looks fine now
- menu looks much better
- still some issues remains
Have a look and test with your GTK3 and favourite theme. Feedback welcomed.

Created attachment 519093
WIP v6

Wrong patch, reuploading.

Download full text (4.4 KiB)

I get this build error:
g++ -o nsWindow.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/hussam/packages/firefox/config/gcc_hidden.h -DNATIVE_THEME_SUPPORT -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD -DOS_LINUX=1 -DOS_POSIX=1 -DCAIRO_GFX -I/home/hussam/packages/firefox/ipc/chromium/src -I/home/hussam/packages/firefox/ipc/glue -I../../../ipc/ipdl/_ipdlheaders -I/home/hussam/packages/firefox/widget/src/gtk2 -I. -I../../../dist/include -I../../../dist/include/nsprpub -I/usr/include/nspr -I/usr/include/nss -I/home/hussam/packages/firefox/widget/src/gtk2/../xpwidgets -I/home/hussam/packages/firefox/widget/src/gtk2/../shared -I/home/hussam/packages/firefox/other-licenses/atk-1.0 -I/home/hussam/packages/firefox/widget/src/gtk2/../shared/x11 -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fomit-frame-pointer -finline-limit=50 -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -DGSEAL_ENABLE -pthread -I/usr/include/gtk-3.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -I/usr/include/gtk-3.0/unix-print -I/usr/include/startup-notification-1.0 -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsWindow.pp /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp
In file included from /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:49:0:
/home/hussam/packages/firefox/widget/src/gtk2/../xpwidgets/nsBaseWidget.h:123:24: warning: ‘virtual gfxASurface* nsBaseWidget::GetThebesSurface()’ was hidden
/home/hussam/packages/firefox/widget/src/gtk2/nsWindow.h:339:18: warning: by ‘gfxASurface* nsWindow::GetThebesSurface(GtkWidget*, cairo_t*)’
/home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp: In function ‘void SetUserTimeAndStartupIDForActivatedWindow(GtkWidget*)’:
/home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1377:5: error: ‘GdkDrawable’ was not declared in this scope
/home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1377:18: error: ‘drawable’ was not declared in this scope
/home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1377:72: error: ‘GDK_DRAWABLE’ was not declared in this scope
/home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1385:62: error: ‘gdk_x11_drawable_get_xdisplay’ was not declared i...

Read more...

It may be caused by some problem in your config files. I'm just finishing a patch which separates gtk3 code to widget/src/gtk3 and allows to build ff with both widgets so it addresses such issues.

Created attachment 519919
WIP v7

Martin separated gtk3 from gtk2 so we now have original widgets/src/gtk2 and new widgets/src/gtk3. He's currently working on plugins (crash when we build with gtk3 and use gtk2 plugin).
Finished some widgets:
- scrollbars
- tabs
- fixed drag&drop but it still flicker when moving.
WIP: custom cursors, gtk3 miss gdkpixmap
Apply patch and build with --enable-default-toolkit=cairo-gtk3.

Is the patch diffed against the RC tarball or latest HG? because I couldn't get it to patch against mozilla-central.

It would be easier to read the patch if it were split into one which did the duplication and another which made the modifications to the new gtk3 dir.

Using "hg copy" would make the duplication easier to follow (and may make splitting the modifications unnecessary).

(In reply to comment #12)
> Is the patch diffed against the RC tarball or latest HG? because I couldn't get
> it to patch against mozilla-central.

It is diffed against changeset 62828:b1ef0685b2e0 (mozilla-central from Feb 18). We don't know how to make changes to our local hg repository and stay synced with mozilla-central in the same time. So we unbitrot patch from time to time.

(In reply to comment #14)
> Using "hg copy" would make the duplication easier to follow (and may make
> splitting the modifications unnecessary).

Hm, I'll check the "hg copy", it looks like good idea.

(In reply to comment #15)
> We don't know how to make changes to our local hg repository and stay
> synced with mozilla-central in the same time.

Getting the equivalent of git commit --amend is not quite trivial with mercurial.
Most people use the mq extension for this. It sounded a bit much effort to me, but, once I actually tried it, it wasn't so hard to learn.

https://developer.mozilla.org/en/Mercurial_Queues

This does come in very handy for separating projects into manageable-sized patches, particularly when each patch in the series is still under development.
The gtk2 plugin work at least should be separate from the base gtk3 patch (and perhaps could even be split into a number of smaller patches).

http://weblogs.mozillazine.org/roc/archives/2010/01/more_on_patch_d.html

I've been doing my best to read the patch since I take particular interest in the native theming portion, but from what I can gather, doesn't GTK3's use of Cairo for drawing widgets mean we can bypass XlibNativeRenderer (and all it's ugly slowness) completely?

I may have missed this, but I don't see you setting the enable-system-cairo configure flag by default when GTK3 is built, which can cause nastiness.

Sure, the code should be optimized and refactorised. Our goal is to create a minimal working patch and then improve it.

(In reply to comment #18)
> I may have missed this, but I don't see you setting the enable-system-cairo
> configure flag by default when GTK3 is built, which can cause nastiness.
Yeah, we missed that. We use enable-system-cairo option of course, it should be probably part of configure.in patch.

22 comments hidden view all 156 comments
Arek Olek (arekolek) wrote :

It is still there in Natty as well.

Marcus Haslam (marcus-haslam) wrote :

I'm out of the office until 1st August.

On 30 Apr 2011, at 19:06, Arek Olek <email address hidden> wrote:

> It is still there in Natty as well.
>
> --
> You received this bug notification because you are a member of
> Papercutters, which is subscribed to One Hundred Paper Cuts.
> https://bugs.launchpad.net/bugs/219385
>
> Title:
>  File filters can make file selection dialog too wide for screen
>
> Status in GTK+ GUI Toolkit:
>  Fix Released
> Status in One Hundred Paper Cuts:
>  Fix Committed
> Status in Ubuntu:
>  Invalid
> Status in “gtk+2.0” package in Ubuntu:
>  Fix Committed
> Status in “gtk+2.0” package in CentOS:
>  New
>
> Bug description:
>  Binary package hint: gnome-desktop-environment
>
>  This bug occurs in Firefox but is likely related to the generic Gnome
>  file-selection-dialog rather than application specific.
>
>  In this case our website is calling OS file dialog via Firefox and
>  JS/Flash, passing in a filter with a description (eg. "Photos and
>  Videos"), and file extension-matching pattern (eg.
>  "*.mov;*.mpg;*.avi;*.jpg")
>
>  Windows dialogs tend to truncate this list, and expand only the drop-
>  down when selected, open and active. Ubuntu/Gnome appears to make the
>  dialog width match the width of the filter drop-down, which is quite
>  long due to the number of file types and mixed-case scenarios (eg.
>  *.jpg;*.JPG)
>
>  Due to the amount of file formats we're filtering based on, the
> dialog
>  box can easily grow wider than the user's display permits.
>
>  See screenshot for illustration:
>  http://farm4.static.flickr.com/3215/2403694395_ca70740726.jpg
>
>  Expected behavior would be to truncate the file filter list display
>  past a certain point to prevent dialog box from growing too large.
>
>  Our testing confirms this behavior in both Ubuntu 7.10 and 8.04RC.
>
>  This bug prevents Ubuntu users from being able to use the advanced
>  upload tool on Flickr.com from Firefox.

98 comments hidden view all 156 comments

Comment on attachment 561999
build fix - startup notification

Make it static inline, like the others.
No need for the GDK_DRAWABLE() cast as GdkWindow is the same type as GdkDrawable.
I'd prefer to drop the GDK_IS_WINDOW check too. I don't think that adds any value.

Created attachment 562004
build fix - startup notification, v2

Fixed. I prefer to keep GDK_DRAWABLE() here, it's the original GTK 2.24 implementation.

Comment on attachment 562004
build fix - startup notification, v2

GDK_DRAWABLE() adds code for an extra function call per caller.

The C type cast is pointless because the C types are the same.

The GType check-instance is pointless because all GDK_TYPE_WINDOW objects are of type GDK_TYPE_DRAWABLE, and gdk_drawable_get_screen will check GDK_IS_DRAWABLE again anyway.

Created attachment 562011
build fix - startup notification, v3

Okay, removed.

Comment on attachment 562011
build fix - startup notification, v3

Thanks.

(In reply to Karl Tomlinson (:karlt) from comment #99)
> GDK_DRAWABLE() adds code for an extra function call per caller.

Actually two function calls: gdk_drawable_get_type and g_type_check_instance.

Comment on attachment 561753
nsWindow.cpp, v6

nsWindow.cpp part: https://hg.mozilla.org/mozilla-central/rev/875cb4f20eac

small intermission from a lurker:

(In reply to Karl Tomlinson (:karlt) from comment #101)
> > GDK_DRAWABLE() adds code for an extra function call per caller.
>
> Actually two function calls: gdk_drawable_get_type and g_type_check_instance.

Only if you don't compile with -DG_DISABLE_CAST_CHECKS which people definitely should do for releases. In that case those casts are just casts.

FWIW, using GNOME coding style, I'd make every cast use the macro both because of style ad because I know there's no performance difference, but huge debuggability improvements (once you turn it on).

Comment on attachment 562011
build fix - startup notification, v3

startup notification part: https://hg.mozilla.org/mozilla-central/rev/2137c5bd3c36

(In reply to Benjamin Otte from comment #103)
> Only if you don't compile with -DG_DISABLE_CAST_CHECKS which people
> definitely should do for releases. In that case those casts are just casts.

We compile with C flags from pkgconfig, which do not seem to include this.
I notice GTK+ does compile with -DG_DISABLE_CAST_CHECKS by default.

I don't see any documentation, but it looks like this could only help.

Seems a little strange to set an undocumented symbol that would change G_TYPE_CHECK_INSTANCE_CAST to not do what it says it does, but all g_type_check_instance_cast would do for us is print a warning (unless G_DEBUG is in the environment). It doesn't return null if the check fails.

Created attachment 563434
gtk drawing patch v.2 + comments

It should address the remarks, my comments are at top of the patch.

Created attachment 563817
mozcontainer.c patch, v2

Comment on attachment 563817
mozcontainer.c patch, v2

>+#include <gdk/gdkx.h>

Surrounding the gdk_x11_* function definitions in gtk2compat.h with
something like "#ifdef GDK_WINDOW_XDISPLAY" would make this include
unnecessary.

>- widget->style = gtk_style_attach (widget->style, widget->window);
>+ gtk_widget_style_attach(widget);

gtk_widget_style_attach is not needed in GTK3, so it may make sense to remove
gtk_widget_style_attach from gtk2compat.h and make the gtk_style_attach line
#ifdef MOZ_WIDGET_GTK2.

>-
>- GTK_WIDGET_SET_FLAGS (widget, GTK_MAPPED);
>+

>-
>- GTK_WIDGET_SET_FLAGS(widget, GTK_REALIZED);
>+

Unnecessary extra white space added in the blank lines.

>+ gtk_widget_set_window(widget, gdk_window_new (gtk_widget_get_parent_window (widget),
>+ &attributes, attributes_mask));

Can you align &attributes with gtk_widget_get_parent_window, please?

Created attachment 564498
mozcontainer.c patch, v3

Fixed comments, ready to commit.

Download full text (4.5 KiB)

Comment on attachment 563434
gtk drawing patch v.2 + comments

>>> if (interior_focus) {
>>>+ GtkBorder border;
>>>+ gtk_style_context_get_border(style, 0, &border);
>>Don't we want to pass the state flags here?
>
> What do you mean?

gtk_style_context_get_border() "Gets the border for a given state [...]" and
uses the "state" parameter rather than the state on the StyleContext. However,
this code is not passing anything for the GtkStateFlags state parameter.

Most, but not all, gtk_style_context_get_border calls in GTK widgets pass the
state flags.

>> gtk_tree_view_column_create_button uses GTK_SHADOW_IN.
>
> gtk_tree_view_column_create_button()? I don't see it anywhere.

http://git.gnome.org/browse/gtk+/tree/gtk/gtktreeviewcolumn.c?id=3.2.0#n876

>> In moz_gtk_tab_paint:
>>+ gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
>> I assume _NORMAL is the default, so this is unnecessary.
>
> I don't see GTK_STATE_FLAG_NORMAL in moz_gtk_tab_paint(), it's only in:
>
> moz_gtk_progressbar_paint
> moz_gtk_progress_chunk_paint
> moz_gtk_tabpanels_paint
> moz_gtk_check_menu_item_paint

I must have pasted something in the wrong place there.

I suspect all the gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL)
calls are unnecessary. The pattern used in GTK is to create the style context
with default state and only ever set the state with a save/restore during
draw. Removing these would save a little code, but I don't feel strongly
about it, if you prefer to leave them in.

>>>+ gtk_render_frame_gap(style, cr,
>>>+ rect->x - gap_loffset,
>>>+ rect->y + gap_voffset - 3 * gap_height,
>>>+ rect->width + gap_loffset + gap_roffset,
>>>+ 3 * gap_height, GTK_POS_BOTTOM,
>>>+ gap_loffset, rect->width);
>>Need to also add gap_loffset here as you did below.
>
> Where the gap_loffset should be?

The last parameter is a position rather than a width, and so needs to be
gap_loffset + rect->width.

>>>+ gtk_render_frame_gap(style, cr,
>>>+ rect->x - gap_loffset,
>>>+ rect->y + rect->height - gap_voffset,
>>>+ rect->width + gap_loffset + gap_roffset,
>>>+ 3 * gap_height, GTK_POS_TOP,
>>>+ gap_loffset, gap_loffset+rect->width);
>> Can you add spaces around the "+", please?
>
> Where? All "+" I see have spaces around.

The last parameter is correct here, but there are no spaces around its "+".

>>+ gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
>> gtk_notebook_paint usually uses gtk_render_frame_gap.
>> Why choose render_frame over render_frame_gap here?
>
> I haven't been able to decrypt the former code logic plus gtk_render_frame_gap()
> does not allow to set negative gap start. gtk_render_frame() seems to work fine
> too but if you have an idea how to transport the gap size to moz_gtk_tabpanels_paint()
> I can modify it.

The gap s...

Read more...

Comment on attachment 564498
mozcontainer.c patch, v3

Backed out for causing build failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d490b0433f

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f09c204296f8

{
In file included from ../../../../widget/src/gtk2/nsWindow.cpp:89:0:
../../../../widget/src/gtk2/gtk2compat.h:114:1: error: expected unqualified-id before '{' token
../../../../widget/src/gtk2/gtk2compat.h: In function 'void gtk_widget_set_can_focus(GtkWidget*, gboolean)':
../../../../widget/src/gtk2/gtk2compat.h:128:79: error: 'container' was not declared in this scope
../../../../widget/src/gtk2/gtk2compat.h:130:79: error: 'container' was not declared in this scope
../../../../widget/src/gtk2/gtk2compat.h: In function 'gboolean gtk_widget_get_visible(GtkWidget*)':
../../../../widget/src/gtk2/gtk2compat.h:134:1: error: redefinition of 'gboolean gtk_widget_get_visible(GtkWidget*)'
../../../../widget/src/gtk2/gtk2compat.h:95:1: error: 'gboolean gtk_widget_get_visible(GtkWidget*)' previously defined here
../../../../widget/src/gtk2/gtk2compat.h: At global scope:
../../../../widget/src/gtk2/gtk2compat.h:113:1: warning: 'void gtk_widget_set_allocation(GtkWidget*, const GtkAllocation*)' declared 'static' but never defined
}

Created attachment 565231
mozcontainer.c patch, v5

Uff, sorry for that, I really should build it on gtk2-2.10 box next time.

Comment on attachment 565231
mozcontainer.c patch, v5

>- widget->window = gdk_window_new (gtk_widget_get_parent_window (widget),
>- &attributes, attributes_mask);
>+ gtk_widget_set_window(widget, gdk_window_new (gtk_widget_get_parent_window (widget),
>+ &attributes, attributes_mask));

Can you align &attributes with gtk_widget_get_parent_window, please?

Created attachment 565464
mozcontainer.c patch, v6, fixed indentation

Fixed indentation.

Comment on attachment 565464
mozcontainer.c patch, v6, fixed indentation

In my queue, which is heading to try first then inbound :-)
https://tbpl.mozilla.org/?tree=Try&rev=a8fbb2a76633

Created attachment 565519
gtk drawing patch v.3

gtk_style_context_get_border uses the state wherever it's available now.

> I suspect all the gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL)
> calls are unnecessary. The pattern used in GTK is to create the style context
> with default state and only ever set the state with a save/restore during
> draw. Removing these would save a little code, but I don't feel strongly
> about it, if you prefer to leave them in.

I prefer to leave GTK_STATE_FLAG_NORMAL here for now, we can remove it / optimize later.
I recall there were issues with rendering when GTK_STATE_FLAG_NORMAL was missing.

> With the API change from a cliprect parameter to a cairo_t, the clip
> rectangles for each half will need to be applied to the cairo_t,
> necessitating the use of cairo_save/restore. I suggest dividing rect into
> left and right halves instead of the approach for gtk2 of dividing the
> cliprect into two halves. (Getting the cliprect from cairo is harder and
> unnecessary.)

> For smaller tabpanels, the logic in the gtk2 port could lead to the gap being
> outside the rect. I suggest just using rect->width as the gap position in the
> render_frame_gap() call for the left half and 0 for the right half.

Updated. I hope I grasp the idea correctly.

Comment on attachment 565464
mozcontainer.c patch, v6, fixed indentation

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f8b4c3c12a

Comment on attachment 565519
gtk drawing patch v.3

It looks like many (but not all) of the changes between attachment 525346 and attachment 540027 have been reverted or lost in this version of the patch.

I don't know whether or not you have a easy way of merging those back in.
It may be easiest to go through attachment 533890 again and check that each issue that you addressed is still addressed.
(Separate vpaned/hpaned functions is fine.)

Created attachment 568338
gkt3drawing, v.4

Uff, you're right, there were some missing parts from previous patch. Some remarks:

>moz_gtk_entry_paint:
>+ //gtk_render_background(style, cr, rect->x + x, rect->y + y, rect->width - 2*x, rect->height - 2*y);
>+ //gtk_render_frame(style, cr, rect->x + x, rect->y + y, rect->width - 2*x, rect->height - 2*y);
> Remove this if it is not used or add a TODO comment explaining why it is kept.
> I can't make much sense of what is happening with the focus_width and the
> rectangles in this function. The old logic looked closer to what we are going
> to need, so I'd prefer to stay with that until this is sorted out.

Removed.

In moz_gtk_treeview_paint:

>- gtk_widget_modify_bg(gTreeViewWidget, state_type,
>- &gTreeViewWidget->style->base[state_type]);
>+ gtk_style_context_get_background_color(style, state_type, &base_col);
>+ gtk_widget_override_background_color(gScrolledWindowWidget, state_type, &base_col);
>
>Is this necessary?
>If so, it looks like it should be applied to the TreeView widget.

Not necessary, removed.

>+ gtk_render_frame(style_tree, cr,
>+ rect->x + xthickness, rect->y + ythickness,
>+ rect->width - 2 * xthickness,
>+ rect->height - 2 * ythickness);
>
>Is this necessary?
>The only gtk_render_frame I see in GtkScrolledWindow is for the rubber band.

Not necessary, removed.

moz_gtk_tab_paint:

> Are you sure about GTK_STATE_FLAG_INSENSITIVE for unselected tabs? Where does
> that come from? I would have expected _NORMAL.

It doesn't matter, changed to _NORMAL.

> moz_gtk_menu_separator_paint:
> + gtk_style_context_add_class(style, GTK_STYLE_CLASS_DEFAULT);
> What is this for?

Some leftover I guess, removed.

Chris Wilson (notgary) on 2011-12-17
Changed in hundredpapercuts:
milestone: lucid-round-2 → precise-9-miscellaneous

Will those patches get merged to any UX build for public testing any time soon? :)

A minor query about
gComboBoxEntryWidget = NULL; /* TODO - gtk_combo_box_entry_new();*/
in patch from comment 122.
What would be wrong with using gtk_combo_box_new_with_entry there ?

Created attachment 588315
review comments on gkt3drawing, v.4

I haven't quite looked at everything you changed here, but most of it, and I'll be on vacation for a week or two, so I'll post what I have.

Created attachment 593982
review comments on gkt3drawing, v.4 (completed)

Comment on attachment 540031
nsLookAndFeel.cpp patch

I've been taking too long to get to these, so I've asked Chris to review these two patches. I'll review an updated gtk3drawing patch, as I've got to know that code quite well.

Created attachment 595369
gtk drawing patch v.5

Thanks, there's an updated version. Some remarks:

> Can you mention removal of the unused cliprect parameters to the TODO list
> please?

Added before moz_gtk_widget_paint().

>The old logic was different from what GetStateFlagsFromGtkWidgetState does,
>which makes me think that GetStateFlagsFromGtkWidgetState should set
>GTK_STATE_FLAG_PRELIGHT (when inHover) and GTK_STATE_FLAG_ACTIVE (when
>depressed or active) independently of each other.

Updated the GetStateFlagsFromGtkWidgetState(), it composes the flags now.

> In moz_gtk_gripper_paint:
> gtk_handle_box_paint also calls gtk_render_frame.

Yeah. gtk_handle_box_paint draws gtk_render_handle() differently,
regards to the handle_position. But the effective_handle_position() (from gtk_handle_box_paint) uses GtkHandleBoxPrivate which seems to be hidden/private.

(In reply to Rafał Mużyło from comment #125)
> A minor query about
> gComboBoxEntryWidget = NULL; /* TODO - gtk_combo_box_entry_new();*/
> in patch from comment 122.
> What would be wrong with using gtk_combo_box_new_with_entry there ?

I expect that would be the correct widget to use. However, I'm guessing it is not as simple as just changing the widget, but other code will also need changing. That can be sorted out separately.

Comment on attachment 595369
gtk drawing patch v.5

Thanks very much.
Just a save/restore pair to remove in moz_gtk_combo_box_paint:

>- gtk_paint_arrow(style, drawable, state_type, shadow_type, cliprect,
>- gComboBoxArrowWidget, "arrow", GTK_ARROW_DOWN, TRUE,
>- real_arrow_rect.x, real_arrow_rect.y,
>- real_arrow_rect.width, real_arrow_rect.height);
>-
>+ style = gtk_widget_get_style_context(gComboBoxArrowWidget);
>+ gtk_style_context_save(style);
>+
>+ gtk_render_arrow(style, cr, ARROW_DOWN,
>+ real_arrow_rect.x, real_arrow_rect.y,
>+ real_arrow_rect.width);

This is no longer applying the state when drawing the arrow, but that seems to
be correct, because I can't see that GtkComboBox passes its state to the style
context for gtk_render_arrow.

> if (!gComboBoxSeparatorWidget)
> return MOZ_GTK_SUCCESS;

>+ gtk_style_context_restore(style);

This save/restore is not balanced when taking the early return, but is
unnecessary anyway because the style context was not changed.

The save/restore for the style context of the gComboBoxSeparatorWidget is
similarly unnecessary.

(In reply to Martin Stránský from comment #129)
> Yeah. gtk_handle_box_paint draws gtk_render_handle() differently,
> regards to the handle_position. But the effective_handle_position() (from
> gtk_handle_box_paint) uses GtkHandleBoxPrivate which seems to be
> hidden/private.

The old gtk2drawing code was apparently only painting the box and not the handle.
i.e. it didn't call gtk_paint_handle.

So perhaps we should drop the gtk_render_handle from moz_gtk_gripper_paint to match that. I don't know where or if this is actually used.

Created attachment 595699
gkt3drawing, v.6

Removed the unnecessary save/restore and gtk_render_handle().

Changed in gtk+2.0 (Ubuntu):
status: Fix Committed → Confirmed
135 comments hidden view all 156 comments
Timothy Arceri (t-fridey) wrote :

The change was made to GTK 3, as Firefox is still a GTK 2 app this issue will remain until the port to GTK3 is finished. I will add the firefox port bug to this bug report.

Also the fix does not expand the file type extensions when the drop down list is selected I have reported this upstream here: https://bugzilla.gnome.org/show_bug.cgi?id=671235

Changed in hundredpapercuts:
status: Fix Committed → Triaged
Changed in firefox:
importance: Unknown → Medium
status: Unknown → In Progress
136 comments hidden view all 156 comments
Micah Gersten (micahg) wrote :

Sorry, as the upstream Mozilla bug has nothing specific to do with this bug report, I'm removing the task, the bug watch can remain though.

no longer affects: firefox
Chris Wilson (notgary) on 2012-06-14
Changed in hundredpapercuts:
milestone: precise-9-miscellaneous → quantal-10-gtk
assignee: Canonical Desktop Team (canonical-desktop-team) → Papercuts Ninja (papercuts-ninja)
Chris Wilson (notgary) on 2013-05-27
Changed in hundredpapercuts:
milestone: quantal-10-gtk → papercuts-s-gtk
Changed in hundredpapercuts:
assignee: Papercuts Ninjas (papercuts-ninja) → nobody
Displaying first 40 and last 40 comments. View all 156 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.