GNOME Bugzilla – Bug 546285
Allow GtkEntry to draw progress
Last modified: 2011-12-30 06:46:08 UTC
Hey! In #epiphany we were wondering about using a progress-in-location-bar idea, while checking that, xan found out this code is available in maemo: https://stage.maemo.org/svn/maemo/projects/haf/trunk/gtk+/gtk/gtkentry.c (hildon_gtk_entry_set_progress_adjustment) This bug is filled so we don't leave the idea behind.
Created attachment 117464 [details] [review] Patch implementing the entry progress with a GtkProgressBar-like API The approach of using a GtkAdjustment for that is IMHO pretty much on crack (and in fact it was a quick demo hack for maemo-gtk that got checked in prematurely). Here is a patch that mirrors GtkProgressBar's API and also allows for an activity mode where the progress indicator bounces forth and back API docs missing, otherwise feels ok-ish to commit.
Hm, some comments. GtkProgress bar uses GTK_STATE_PRELIGHT to draw the "bar", so it may be worth considering to copy that. (The buildin gtkrc might need to be modified though.) Also, I think it would make sense to draw text that is on top of the progress bar differently to normal text. As the "bar" is drawn with a box (so will use bg[XXX]) the text on top of it should be drawn with fg[XXX] in my oppinion.
(In reply to comment #1) > Created an attachment (id=117464) [edit] > Here is a patch that mirrors GtkProgressBar's API and also allows for > an activity mode where the progress indicator bounces forth and back > > API docs missing, otherwise feels ok-ish to commit. Very nice, I like the API, it's simple and sufficient. You didn't add any properties, though. I'd appreciate if you coud make "fraction" a property.
How about a GtkProgress interface instead, that GtkProgressBar and GtkEntry then would implement?
We already have a (deprecated) GtkProgress parent class of GtkProgressBar and thus can't really use that as an interface name. I would prefer to re-use that proper name as an interface once the deprecated stuff has been removed in GTK 3.x; instead of using another (articifial) name just to be different from GtkProgress.
Coincidentally I was experimenting with something like that only a few days before this patch came up. In my use case I don't want to see progress if fraction is 1.0. I figured I'd mention it since the patch draws anything above 0.0.
I take that back actually. It is easy enough to special case 1.0 in applications while not showing 1.0 in the widget would make it impossible to show 100% if someone wants it for their use case.
(In reply to comment #1) > Created an attachment (id=117464) [edit] > Patch implementing the entry progress with a GtkProgressBar-like API > > The approach of using a GtkAdjustment for that is IMHO pretty much on crack > (and in fact it was a quick demo hack for maemo-gtk that got checked in > prematurely). > > Here is a patch that mirrors GtkProgressBar's API and also allows for > an activity mode where the progress indicator bounces forth and back > Thanks a lot Mitch, after the API freeze, this looks good to go in for me. I just have minor comments: >+ guint progress_pulse_mode : 1; >+ guint progress_pulse_way_back : 1; There's still room for these two bits in GtkEntryPrivate around change_count, so we don't need to waste an extra int. >+gtk_entry_set_progress_fraction (GtkEntry *entry, It'd be really nice here to do: if (fabs (fraction - old_fraction) > 0.0001 || ...pulse_changed...) gtk_entry_queue_draw (entry); That way we don't waste CPU cycles if we get fine grained progress information from the application. (I picked epsilon small enough for an 8192 pixel screen.) >+++ tests/testgtk.c (working copy) Thanks for the test code, it'd be great to actually have this as exemplary code in gtk-demo though. > API docs missing, otherwise feels ok-ish to commit. Please provide a new patch with API docs and the above issues fixed then ;)
Created attachment 118372 [details] [review] Updated patch
(In reply to comment #9) > Created an attachment (id=118372) [edit] > Updated patch Tested the patch, works fine so far. Since you don't comment on your update, I take it you are still working on theming (text colour) and properties? Looking forward to this feature.
Created attachment 118572 [details] [review] Add properties Same patch but with properties added. The drawing of the text on top of the progress is still a problem. Maye it should simply be done exactly the same way as in GtkProgress.
(In reply to comment #11) > Created an attachment (id=118572) [edit] > Add properties > > Same patch but with properties added. The drawing of the text on top > of the progress is still a problem. Maye it should simply be done exactly > the same way as in GtkProgress. Sorry to be picking the nits but you forgot the g_object_notify's in both accessors.
Created attachment 118575 [details] [review] Add notifications Indeed, thanks :) fixed in this version.
Created attachment 119499 [details] [review] [PATCH] GtkEntry progress v4.1 gtk/gtk.symbols | 5 + gtk/gtkentry.c | 289 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- gtk/gtkentry.h | 12 +++ tests/testgtk.c | 72 ++++++++++++++- 4 files changed, 375 insertions(+), 3 deletions(-)
The patch is only an update to build with trunk. Nothing else.
Created attachment 119503 [details] Screenshot A couple of comments: - there's a white line/border, progress bar don't have that and I think it looks ugly, like if it was just an unpolished thing that we didn't care fixing - the text should turn select[fg] when the bg to that text is the progress bar fill. Like when you select text, the text fg color is changed, usually -if not always- select[bg] and normal[fg] don't have good contrast between them. In clearlooks-speak: make the text white when the background is blue because of progress. - I would have expected a more plain fill, like simply a color, not the full progress bar look (in the screenshot, the / / bars), opinions?
Created attachment 119504 [details] Looks proposal Completing my previous comment, I think the 4th bar in this gimp'd screenshot would look lots nicer that the current one (1st bar). The 3rd bar is the 1st one + white fg, but I think the / / background of the progress animation makes the text more difficult to read. The rationale for not seeing the progress bar animation as a good background is that progress bar fill is thought for little or no text, while the selection background/fill is thought for lots of text (hence, offers a much better contrast always).
Created attachment 119507 [details] [review] [PATCH] Don't draw using "bar" look gtk/gtkentry.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Diego, how exactly the progress bar will look like will depend on the theme. I think that GTK+ should primarily make sure that the detail strings, etc. are sane and good theming can be done. Setting the detail string to NULL is a bad idea, imo. One could use a new detail strings (other than "bar") though. About the white line. You would need to draw the "bar" twice (for each of the two windows). One may also need a style property to define the border around the bar.
I don't find the styling of the bar bad at all, and I agree in any case it's up to the theme. I assume with 'white line' you mean the space between the bar and the borders? Might be worth making a mockup as well. I for instance would not have thought of that as a mistake in the first place if you hadn't said so.
(In reply to comment #18) > Created an attachment (id=119507) [edit] > [PATCH] Don't draw using "bar" look Like Benjamin said, using no detail is quite bad. However changing it might be useful so themes can adjust entry progress bars particularly. Mitch, can the detail be changed from "bar" to e.g. "entry-progress"?
(In reply to comment #13) > Created an attachment (id=118575) [edit] > Add notifications Thanks for the patch, looks ready for application to me, except for using specific details as mentioned in comment #21.
Fixed in SVN: 2008-12-05 Michael Natterer <mitch@imendio.com> Bug 546285 – Allow GtkEntry to draw progress * gtk/gtkentry.[ch]: add new API similar to GtkProgressBar which allows to set the entry's progress_fraction, its progress_pulse_step and to let the entry's progress pulse. * gtk/gtk.symbols: updated. * tests/testgtk.c: add progress demo code to the "Entry" window.
(In reply to comment #4) > How about a GtkProgress interface instead, that GtkProgressBar and GtkEntry > then would implement? (In reply to comment #5) > We already have a (deprecated) GtkProgress parent class of GtkProgressBar > and thus can't really use that as an interface name. > > I would prefer to re-use that proper name as an interface once the > deprecated stuff has been removed in GTK 3.x; instead of using > another (articifial) name just to be different from GtkProgress. This sounds sane, so... what's the bug number for that? ;)
(In reply to comment #23) > Fixed in SVN: > > 2008-12-05 Michael Natterer <mitch@imendio.com> > > Bug 546285 – Allow GtkEntry to draw progress > > * gtk/gtkentry.[ch]: add new API similar to GtkProgressBar which > allows to set the entry's progress_fraction, its progress_pulse_step > and to let the entry's progress pulse. > > * gtk/gtk.symbols: updated. > > * tests/testgtk.c: add progress demo code to the "Entry" window. You never have feedback with regard to the text colour, and apparently you didn't change it. Reopening to clarify this, see previous comments.
Another unsolved color problem is the conflict between selection and progress.
*** Bug 566086 has been marked as a duplicate of this bug. ***
*** Bug 568106 has been marked as a duplicate of this bug. ***
Created attachment 129080 [details] [review] Themable progress area inside the entry This patch makes the border around the progress bar themable. It is also drawn underneath all content, including the icon, with this patch.
I think the patch I just attached would improve the themability for eg. Clearlooks, because one can get rid of the white border around the progress bar.
Oh, I just relized something. Currently the progress bar is drawn with a draw_box call with the state SELECTED. This means that it makes sense to use the bg[SELECTED] color, but the text is still drawn in text[NORMAL]. I really do think that the text should be drawn using fg[SELECTED] with the current way of drawing the bar.
Created attachment 129138 [details] [review] progress area + text color change This patch is based on the last patch, but also changes the text color as mentioned earlier.
Hi, this seems to work pretty well, although it adds a few: (epiphany:11666): Gdk-CRITICAL **: gdk_drawable_get_size: assertion `GDK_IS_DRAWABLE (drawable)' failed The problem, I think, is that set_progress_fraction can be called with the widget being unrealized, but it will call get_progress_area which assumes widget->window exists. Probably the function should be refactored a bit to take this into account.
Looks good, in general. The one big problem that is still not addressed is that progress and selection can not be told apart.
I don't think you want to replace gtk_entry_queue_draw with gtk_widget_queue_draw in those places.
Some more deficiencies of the current progress drawing approach: - it doesn't take into account focus (ie compare focused/nonfocused selection vs focused/nonfocused progress) - it doesn't render differently in insensitive entries
I still think we should have separate colors for progress/insensitive progress
> The one big problem that is still not addressed is that progress and selection > can not be told apart. Hm, I don't see how this could be improved much in GTK+. The theme is able to select different colors for the selection and bar. (Bar is bg[XXX] selection is base[XXX] > I don't think you want to replace gtk_entry_queue_draw with > gtk_widget_queue_draw in those places. This needs to be done, because the style property allows the progress bar to be drawn over all areas of the entry, not only the text part. - it doesn't take into account focus (ie compare focused/nonfocused selection vs focused/nonfocused progress) - it doesn't render differently in insensitive entries Hm, we could switch the drawing of the text and progress bar between the ACTIVE/SELECTED/INSENSITIVE states. ACTIVE would be focused, SELECTED normal and INSENSITIVE for insensitive entries. I expect that the ACTIVE state will look ugly with pretty much all themes currently, but at least the GNOME themes could be fixed for 2.26.
> Hm, I don't see how this could be improved much in GTK+. The theme is able to > select different colors for the selection and bar. (Bar is bg[XXX] selection is > base[XXX] Well, we have symbolic colors now. I don't think we can solve this with the restricted set of colors thats available in GtkStyle > This needs to be done, because the style property allows the progress bar to be > drawn over all areas of the entry, not only the text part. At the very least, you need the GTK_WIDGET_DRAWABLE check that gtk_entry_queue_draw has. Oh, btw, I don't think progress-border really adds anything. I'd be fine with just always drawing without a border
(In reply to comment #39) > Well, we have symbolic colors now. I don't think we can solve this with the > restricted set of colors thats available in GtkStyle Hm, could work. Not sure what would be the best way. > > This needs to be done, because the style property allows the progress bar to be > > drawn over all areas of the entry, not only the text part. > > At the very least, you need the GTK_WIDGET_DRAWABLE check that > gtk_entry_queue_draw has. Yes, some checks need to be added. > Oh, btw, I don't think progress-border really adds anything. I'd be fine with > just always drawing without a border This style property is actually needed for eg. Clearlooks, because it draws a shadow around the whole entry. Just drawing the bar smaller does not have the same effect as subtracting the padding in GTK+.
Created attachment 129341 [details] [review] New patch, adding insensitive state and sanity checks Not changed in this patch is the focused vs. non-focused state. I am not sure anymore this even makes sense, the "state" of the progress bar does not change, just the one of the text selection. Oh, I get warnings for lrint. I guess math.h may only be in c99 and not c89? The reason I added it was that the bar was 1 pixel too small when the fill_level was 1.0 for some reason. But I guess that one could eg. add 0.5 instead.
Ping, what is the status? I would love to see this patch going into 2.16.0, with the corresponding support in gtk-engines. And time is running out with hard code freeze just over a week away. If neccessary I will do some updates the patch, but unfortunately exams are comming up ... Support in gtk-engines/gnome-themes will likely happen till monday.
Here is my take on this: 1) progress-border and drawing all the way to the edge/under icons is good 2) should be able to necessary rounding without lrint (all of the rest of GTK+ does) 3) I think that using the same color as the selection and inverting the text color on progress is wrong. We should instead a) pick a different, lighter color for the progress and b) not change the text color Picking a different color for the default theme could be done by doing some shading and mixing based on the selection color. Optionally, it could involve some symbolic color. Or we could leave it up to theme engines to find a suitable color themselves.
And I forgot 4) insensitive rendering is good
Created attachment 129804 [details] [review] default theme change, some improvements * patch now uses floor/ceil instead of lrint, etc. * bar is drawn after the shadow, but before the focus (interior-focus = 0) * text is only drawn twice if really necessary * default theme is adjusted, so that it looks a bit better. I am not sure about the text color anymore. At first I thought it was a good idea to use bg[], but now I am pretty much indifferent. I see advantages in both approaches. Separate text color: * Pretty much all themes need to be modified so that selections are visible * one would use bg[] for the bar and fg[] for the text One text color: * Most engines need to be modified to select a good background color * Choosing a specific color instead of a generated one would need a symbolic color or an engine option
Here is one more thing that goes wrong when flipping the text color: unlike the selection, you can have the text cursor inside the progress, and its color does not get flipped. So you get white text with a black cursor...
We should probably try to get the cursor-on-progress color right, still. But this is a sufficient improvement that I am committing it now. 2009-03-02 Matthias Clasen <mclasen@redhat.com> Bug 546285 – Allow GtkEntry to draw progress * gtk/gtkentry.c: Improve the drawing of progress in entries, using fg/bg[SELECTED]. Add a progress-border style property. Draw progress behind icons too. * gtk/gtkrc.c: Add defaults for fg/bg[SELECTED] in entries. Patch by Benjamin Berg.