Linux: add icon and description preview to app shortcut dialog

Linux: add icon and description preview to app shortcut dialog

TEST=none
BUG=42892
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52412

[Paweł Hajdan Jr., 2010-07-06 19:34:31]

This does not yet deal with the case when the description does not fit in the
fixed-size white box. How should I deal with such situation? Elide the text
somehow, or adjust the size of the box? Is there already some code I can take a
look at to see how it should be done?

Also, the amount of GTK code in this file is now bigger. I wonder if it wouldn't
be nice to separate some parts of it to private methods or helpers in the
anonymous namespace. What do you think?

[Finnur, 2010-07-06 22:05:03]

Is this value overridden in the non-US locale files (ie. locale_settings_ar)?

If so, won't those values need to be updated as well?

[Paweł Hajdan Jr., 2010-07-07 12:48:59]

Finnur, thanks for the drive-by comment. I've done a quick sanity check:

~/chromium/src $ git grep IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS
chrome/app/resources/locale_settings.grd:      <message
name="IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS" use_name_for_id="true">
chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:                  
                    IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS,
chrome/browser/views/create_application_shortcut_view.cc:  // TODO(evanm):
should this use IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS?

So it's only defined in one .grd file.

[Finnur, 2010-07-07 14:32:18]

Excellent, thanks. I saw this review fly by while using my phone, so
following up was a little painful. :)

On Wednesday, July 7, 2010,  <phajdan.jr@chromium.org> wrote:
> Finnur, thanks for the drive-by comment. I've done a quick sanity check:
>
> ~/chromium/src $ git grep IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS
> chrome/app/resources/locale_settings.grd:      <message
> name="IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS" use_name_for_id="true">
> chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:
>                     IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS,
> chrome/browser/views/create_application_shortcut_view.cc:  // TODO(evanm):
> should this use IDS_CREATE_SHORTCUTS_DIALOG_WIDTH_CHARS?
>
> So it's only defined in one .grd file.
>
> http://codereview.chromium.org/2847050/show
>

[Evan Martin, 2010-07-07 15:49:09]

http://codereview.chromium.org/2847050/diff/1/3
File chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc (right):

http://codereview.chromium.org/2847050/diff/1/3#newcode32
chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:32: const char*
kBoxBackgroundColor = "white";
IIRC const char[] instead saves an indirection.

But more generally, setting explicit colors here is dangerous because people can
have themes that are (for example) white-on-black and this box will look ugly. 
Perhaps it'd be better to leave the colors alone and instead use a GtkFrame to
put a border around the highlighted content?

http://codereview.chromium.org/2847050/diff/1/4
File chrome/browser/gtk/create_application_shortcuts_dialog_gtk.h (right):

http://codereview.chromium.org/2847050/diff/1/4#newcode57
chrome/browser/gtk/create_application_shortcuts_dialog_gtk.h:57: // Information
about shortcut to be created.
This comment says more or less the same thing as the variable name.  What is
this information?  Or maybe just call it 'ShortcutInfo' rather than the vague
'Information' to tell me that I need to look at the ShortcutInfo header to know
what this contains?

[Paweł Hajdan Jr., 2010-07-07 16:10:19]

http://codereview.chromium.org/2847050/diff/1/3
File chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc (right):

http://codereview.chromium.org/2847050/diff/1/3#newcode32
chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:32: const char*
kBoxBackgroundColor = "white";
On 2010/07/07 15:49:09, Evan Martin wrote:
> IIRC const char[] instead saves an indirection.
> 
> But more generally, setting explicit colors here is dangerous because people
can
> have themes that are (for example) white-on-black and this box will look ugly.

> Perhaps it'd be better to leave the colors alone and instead use a GtkFrame to
> put a border around the highlighted content?

It seems that the Windows code does the same thing: hardcodes the colors. I
asked the UI team leads about that, and they told me to just copy the Windows
UI.

[Elliot Glaysher, 2010-07-07 16:15:36]

We already break with the windows ui in cases where they set the color of
text explicitly. We should do the same here.

On Jul 7, 2010 9:10 AM, <phajdan.jr@chromium.org> wrote:
>
> http://codereview.chromium.org/2847050/diff/1/3
> File chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc
> (right):
>
> http://codereview.chromium.org/2847050/diff/1/3#newcode32
> chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:32: const
> char* kBoxBackgroundColor = "white";
> On 2010/07/07 15:49:09, Evan Martin wrote:
>> IIRC const char[] instead saves an indirection.
>
>> But more generally, setting explicit colors here is dangerous because
> people can
>> have themes that are (for example) white-on-black and this box will
> look ugly.
>> Perhaps it'd be better to leave the colors alone and instead use a
> GtkFrame to
>> put a border around the highlighted content?
>
> It seems that the Windows code does the same thing: hardcodes the
> colors. I asked the UI team leads about that, and they told me to just
> copy the Windows UI.
>
> http://codereview.chromium.org/2847050/show

[Evan Martin, 2010-07-07 16:15:40]

On 2010/07/07 16:10:19, Paweł Hajdan Jr. wrote:
> It seems that the Windows code does the same thing: hardcodes the colors. I
> asked the UI team leads about that, and they told me to just copy the Windows
> UI.

They don't know what they are talking about in this case, though.  In general
the answer to all questions is "copy what Windows does", and that works up until
the point where it makes sense to diverge.  I can talk with them about it on
your behalf if you'd like.  :)

[Paweł Hajdan Jr., 2010-07-07 16:19:19]

On Wed, Jul 7, 2010 at 18:15, <evan@chromium.org> wrote:

> On 2010/07/07 16:10:19, Paweł Hajdan Jr. wrote:
>
>> It seems that the Windows code does the same thing: hardcodes the colors.
>> I
>> asked the UI team leads about that, and they told me to just copy the
>> Windows
>> UI.
>>
>
> They don't know what they are talking about in this case, though.  In
> general
> the answer to all questions is "copy what Windows does", and that works up
> until
> the point where it makes sense to diverge.  I can talk with them about it
> on
> your behalf if you'd like.  :)


Yes, let's keep the UI team in the loop. I'm happy with whatever decision
that produces.

[Evan Stade, 2010-07-07 21:27:42]

http://codereview.chromium.org/2847050/diff/1/3
File chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc (right):

http://codereview.chromium.org/2847050/diff/1/3#newcode32
chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:32: const char*
kBoxBackgroundColor = "white";
I had this same problem with the extensions install permissions dialog. The
solution is to stuff everything into a GtkTextView (which "usually" has a white
background, but always has a theme-aware background). The only additional
difficulty would be getting the image in there and indenting the text.

Alternately you could try to extract the textview background color from the
theme.

http://codereview.chromium.org/2847050/diff/1/3#newcode133
chrome/browser/gtk/create_application_shortcuts_dialog_gtk.cc:133: if
(!shortcut_info_.description.empty()) {
nit: ternary operator would make this more clear + concise

[Paweł Hajdan Jr., 2010-07-14 15:37:57]

Updated code to not hardcode colors, and applied estade's suggestion about
ternary operator.

Please take another look.

[Evan Martin, 2010-07-14 18:02:52]

LGTM

[Evan Stade, 2010-07-14 18:19:11]

lgtm too