Created attachment 285777 [details] [review]
Use GdTwoLinesRenderer for location entry completion

The grey subtitle (URL) of completions in the location entry dropdown
clash badly with the selected blue background. Instead of forcing this
text to be a particular color, use the custom cell renderer libgd provides
for this situation.

While this is much more readable than before, it would be better if the
subtitle changed to white when selected.

Carlos, what should we do here? This patch makes the subtitle of each completion result look much better when selected than what we have now, but the dep on libgd seem excessive (and I think it's supposed to be private to libgd, to the extent that matters for a submodule).

We can simply copy-paste the cell renderer implementation to lib, instead of depending on all gd, I think we already have some gd code copy pasted in ephy. Could you provide a screenshot?

Created attachment 287674 [details]
Before

Not sure if there's benefit to copying anything out of libgd; that just makes it a bit harder to update.  libgd is designed so that only the files we select are compiled and even distributed.

Comparing these screenshots, it looks like GdTwoLinesRenderer uses more vertical whitespace than we do.

Created attachment 287675 [details]
After (note you can read the selected URL now)

I don't see much difference, not sure the change is worth it

Hm, for me, the first is almost impossible to read, but the second is fairly readable (not great, but much better).  Maybe it depends on your monitor?

I should mention that I'm referring to the gray subtitle, not the white title. Looking at that screenshot again, the page URL is showing up where the page title belongs. That's a bug in the history service.

(In reply to comment #3)
> We can simply copy-paste the cell renderer implementation to lib, instead of
> depending on all gd, I think we already have some gd code copy pasted in ephy.

We had for the overview, but we removed it when the HTML implementation landed. I agree with you that we can copy/paste though.

Wait, why do we need this in the first place? :)

We don't really; anything that makes the subtitle readable would suffice.

Created attachment 296053 [details] [review]
Use GdTwoLinesRenderer for location entry completion

Here's a version that copypastes into ephy, avoiding the dep on libgd. It makes
a significant difference on both my laptop and my desktop monitor so it'd be
nice to get some variant on this committed.

I still think this adds more code to maintain for very little benefit. At least I don't see a huge difference, but if this a problem for users I'm not opposed to the change.

I would prefer to use the first patch that adds libgd as a submodule, so it's not our code to maintain.

Dependencies also require maintenance work, specially when there's no backwards API/ABI compatibility

(In reply to comment #14)
> Dependencies also require maintenance work, specially when there's no backwards
> API/ABI compatibility

That's why it's a submodule, so we don't need to update to a new version of libgd unless there's an actual change in GdTwoLinesRenderer that we want.

Comment on attachment 296053 [details] [review]
Use GdTwoLinesRenderer for location entry completion

Obsoleting; adding this much code just to fix a color is excessive.

I still think libgd is an appropriate dependency, since GdTwoLinesRenderer was created for *exactly* this use, but I can see why you're both unenthusiastic.

Anyway I think this is wrong:

style = gtk_widget_get_style_context (entry);
gtk_style_context_get_color (style, GTK_STATE_FLAG_INSENSITIVE,
			     &color);

For some reason that looked OK to me back in September when I filed this bug, but... it's just not right.

Yosef, if you apply the patch at [1] AFTER applying all the UTF-8 patches, I think you might be able to avoid the crash you reported. (At least, this patch removes the code where the crash occurred in the previous patch.)

[1] http://pkgs.fedoraproject.org/cgit/epiphany.git/plain/0012-Use-GdTwoLinesRenderer-for-location-entry-completion.patch?id=d5c5e34e48badbe02fd5c667db0f3f102e2356a2

Guys, me designer, so no idea what the patch does, anyway, the problem here is that the subtitle remains grey when selected. No idea how is that implemented, but the subtitle should have the dim-label style applied (istead of insensitivizing?), so when the cell gets selected it turns to white as the main title does (dimmed by the stleclass though) remaining visible. Hope this is clear enough.

Note: the dim label class just sets the alpha of the object to 0.55, in case it's not possible to have the css magic working, you can pick the selected color text and set the label alpha to 0.55 (opacity: 0.55, css speaking), if that doesn't/couldn't work you could set the alpha on the color directly (gtkcss speaking that color is "alpha(whatevercolor, .55)", in case you can't do this as well, as a last extent you could just make the subtitle maching the text color when selected forgetting about the dimming, the important thing here is to have that text visible.

See bug #735966 for why dim-label is no longer used

You could just do the same then as in that bug then, no?

That's the code we use here. The problem is that dim-label in Adwaita 3.14+ works by setting the opacity of the label rather than changing the color of the font, but you can't get opacity from a GtkStyleContext.

yup that's what dim-label does, we don't set a color w/o full alpha since it breaks symbolic icons, basically theme wise the translucent color was a hack, setting opacity is the proper solution.

Lapo took care of this with his new theme, changing them to black (not ideal, but waaaay better than it was before).

Reopening. I can't read subtitles again, and I want to be able to change the color of the subtitle based on whether or not the completion item is selected. GdTwoLinesRenderer is exactly what we need.

Bastien, I'm kind of abusing the public/private module split here, by including _view-common instead of one of the public view modules:

LIBGD_INIT([_view-common])

The reason for this is to avoid distributing and compiling a bunch of files that we don't need, since I'm only using GdTwoLinesRenderer. I'd rather not copy GdTwoLinesRenderer into Epiphany directly, since it's nice to be able to easily get fixes from your repo. I figure if you ever remove GdTwoLinesRenderer or _view-common, we can simply not update libgd.

Sound good?

Created attachment 317738 [details] [review]
Use GdTwoLinesRenderer to display the location entry completion

Accordingly, we get to remove Carlos's horrible hack adding markup to
the completion model (which in MVC should only model, not contain
style), and Claudio's horrible hack adding a bool property to make the
markup optional (to avoid breaking the search provider, which also uses
the completion model).

The markup was originally moved to the completion model in order to
remove the cell data function, which was being run continuously due to
some bug. Because GdTwoLinesRenderer uses real cell attributes to
display the title and url separately, we no longer need to worry at all
about the task it used to perform -- merging the title and url into one
string, with a newline and formatting markup to control size and color
of the second line. GdTwoLinesRenderer takes care of this much more
cleanly.

The only user-visible change is that it is now possible to read URLs in
the completion when the row is selected, as the gray is darker.
Apparently some people can read it fine, but the color was too similar to
the selection blue on the three monitors I tested.

Created attachment 317741 [details] [review]
Use GdTwoLinesRenderer to display the location entry completion

Accordingly, we get to remove Carlos's horrible hack adding markup to
the completion model (which in MVC should only model, not contain
style), and Claudio's horrible hack adding a bool property to make the
markup optional (to avoid breaking the search provider, which also uses
the completion model).

The markup was originally moved to the completion model in order to
remove the cell data function, which was being run continuously due to
some bug. Because GdTwoLinesRenderer uses real cell attributes to
display the title and url separately, we no longer need to worry at all
about the task it used to perform -- merging the title and url into one
string, with a newline and formatting markup to control size and color
of the second line. GdTwoLinesRenderer takes care of this much more
cleanly.

The only user-visible change is that it is now possible to read URLs in
the completion when the row is selected, as the gray is darker.
Apparently some people can read it fine, but the color was too similar to
the selection blue on the three monitors I tested.

(In reply to Michael Catanzaro from comment #26)
> Bastien, I'm kind of abusing the public/private module split here, by
> including _view-common instead of one of the public view modules:
> 
> LIBGD_INIT([_view-common])
> 
> The reason for this is to avoid distributing and compiling a bunch of files
> that we don't need, since I'm only using GdTwoLinesRenderer. I'd rather not
> copy GdTwoLinesRenderer into Epiphany directly, since it's nice to be able
> to easily get fixes from your repo. I figure if you ever remove
> GdTwoLinesRenderer or _view-common, we can simply not update libgd.
> 
> Sound good?

It's not really "my repo", but this renderer is already used in Videos, Photos, Documents, so it's probably going to keep up with GTK+ versions.

Would be good to start thinking about having such a renderer directly in GTK+, though the other apps are more likely to start using GtkFlowBox rather than keeping on using such hacks.

Good enough to fix that problem though.

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