Bug 552662 - Find action should use Search style
Summary: Find action should use Search style
Status: REOPENED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.14   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Andrew Obuchowicz CLA
QA Contact:
URL:
Whiteboard: 4.15 M2
Keywords: usability
Depends on:
Blocks: 558961
  Show dependency tree
 
Reported: 2019-11-04 09:23 EST by Lars Vogel CLA
Modified: 2020-11-30 05:45 EST (History)
8 users (show)

See Also:


Attachments
Find actions with cancel icon using dark GTK theme (61.70 KB, image/png)
2020-01-09 09:51 EST, Andrew Obuchowicz CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Vogel CLA 2019-11-04 09:23:48 EST
Our search widgets typically use the SWT.SEARCH style bit with ICON_CANCEL. I suggest to use this also for the Find Action dialog.
Comment 1 Lars Vogel CLA 2019-11-04 09:24:34 EST
Mickael, WDYT?
Comment 2 Mickael Istria CLA 2019-11-04 09:38:32 EST
I don't have an opinion here, as I'm not sure what this would actually change nor where you'd like to apply this.
But I'm find in principle in adding the "Search" semantic flag to relevant widgets.
Comment 3 Lars Vogel CLA 2019-11-04 09:42:28 EST
We would get an Cancel icon at the right if the user entered text (see Open Type)
Comment 4 Mickael Istria CLA 2019-11-04 09:44:48 EST
(In reply to Lars Vogel from comment #3)
> We would get an Cancel icon at the right if the user entered text (see Open
> Type)

Ok, that would make sense.
Comment 5 Lars Vogel CLA 2019-11-15 02:53:09 EST
Patrick, something for you?
Comment 6 Lars Vogel CLA 2019-11-27 07:52:29 EST
(In reply to Lars Vogel from comment #5)
> Patrick, something for you?

Ping?
Comment 7 Eclipse Genie CLA 2020-01-06 16:20:22 EST
New Gerrit change created: https://git.eclipse.org/r/155335
Comment 9 Eclipse Genie CLA 2020-01-09 08:24:17 EST
New Gerrit change created: https://git.eclipse.org/r/155543
Comment 10 Andrey Loskutov CLA 2020-01-09 08:25:59 EST
(In reply to Eclipse Genie from comment #9)
> New Gerrit change created: https://git.eclipse.org/r/155543

Revert of original commit, because it caused bug 558961.

I wonder on which system this was validated, if it looks ugly on both GTK/Windows.
Comment 11 Andrew Obuchowicz CLA 2020-01-09 09:51:39 EST
Created attachment 281433 [details]
Find actions with cancel icon using dark GTK theme

(In reply to Andrey Loskutov from comment #10)
> (In reply to Eclipse Genie from comment #9)
> > New Gerrit change created: https://git.eclipse.org/r/155543
> 
> Revert of original commit, because it caused bug 558961.
> 
> I wonder on which system this was validated, if it looks ugly on both
> GTK/Windows.

It was validated on GTK. At the time of writting the patch I didn't notice the issue with the margins, and it looks slighly better with Adwaita-Dark GTK theme compared to other GTK themes (see screenshot attached).

Regardless I should have been more critical of how the margins were affected. Especially with a light GTK theme.

Will be looking into a fix.
Comment 12 Lars Vogel CLA 2020-01-09 10:07:09 EST
Reverting minor UI issues before someone can have a look at it, seems like a overeaction. I personally don't even notice the margins but having the cancel icon is really helpful (to me).
Comment 13 Andrey Loskutov CLA 2020-01-09 10:37:07 EST
(In reply to Lars Vogel from comment #12)
> Reverting minor UI issues before someone can have a look at it, seems like a
> overeaction. I personally don't even notice the margins but having the
> cancel icon is really helpful (to me).

It looks just ugly now on GTK, for two themes, Adwaita and Clearlooks (also on Windows), so why it is overreacting to revert something that makes things worse as before? It is not about margins, it is about that border around text, and the resulting look.
Comment 14 Lars Vogel CLA 2020-01-09 10:56:59 EST
(In reply to Andrey Loskutov from comment #13)
> (In reply to Lars Vogel from comment #12)
> > Reverting minor UI issues before someone can have a look at it, seems like a
> > overeaction. I personally don't even notice the margins but having the
> > cancel icon is really helpful (to me).
> 
> It looks just ugly now on GTK, for two themes, Adwaita and Clearlooks (also
> on Windows), so why it is overreacting to revert something that makes things
> worse as before? It is not about margins, it is about that border around
> text, and the resulting look.

Because we are still in development and not before a release so we not in a rush and "but this is just about L&F, not a functional issue." to quote yourself from https://bugs.eclipse.org/bugs/show_bug.cgi?id=558953#c4
Comment 15 Andrew Obuchowicz CLA 2020-01-09 11:38:30 EST
(In reply to Lars Vogel from comment #12)
> Reverting minor UI issues before someone can have a look at it, seems like a
> overeaction. I personally don't even notice the margins but having the
> cancel icon is really helpful (to me).

I appreciate the support on the patch Lars (as mentioned I didn't notice the margins either).

I should add, however, that I'm totally fine with Andrey reverting this. Had I personally noticed the issue prior to submitting the patch, I would have waited till I found a proper fix to the margin issues.
Comment 16 Lars Vogel CLA 2020-01-09 11:52:47 EST
(In reply to Andrew Obuchowicz from comment #15)

> I should add, however, that I'm totally fine with Andrey reverting this. Had
> I personally noticed the issue prior to submitting the patch, I would have
> waited till I found a proper fix to the margin issues.

IMHO we should reserve reverts for real issue and not more minor UI glitches.

Reverts send IMHO the strong message of "this was not acceptable and must be removed. Fast!". I prefer to give the developer time to invest if the issue can be fixed without the revert. If the person does not find a way or does not react we still can revert.
Comment 17 Andrey Loskutov CLA 2020-01-10 04:02:12 EST
(In reply to Lars Vogel from comment #16)
> (In reply to Andrew Obuchowicz from comment #15)
> 
> > I should add, however, that I'm totally fine with Andrey reverting this. Had
> > I personally noticed the issue prior to submitting the patch, I would have
> > waited till I found a proper fix to the margin issues.
> 
> IMHO we should reserve reverts for real issue and not more minor UI glitches.
> 
> Reverts send IMHO the strong message of "this was not acceptable and must be
> removed. Fast!". I prefer to give the developer time to invest if the issue
> can be fixed without the revert. If the person does not find a way or does
> not react we still can revert.

If you look on the patch, there isn't much to be "fixed" except to revert it. And I meant it is not worth the effort to retrigger the M1 rebuild only because of this one issue. Now we have a bigger problem and rebuild request anyway, I will merge reverted patch.
Comment 19 Lars Vogel CLA 2020-01-10 05:01:19 EST
(In reply to Andrey Loskutov from comment #17)

> If you look on the patch, there isn't much to be "fixed" except to revert
> it. 


Fixing the margins would IMHO be the right fix.
Comment 20 Andrey Loskutov CLA 2020-01-10 05:07:38 EST
(In reply to Lars Vogel from comment #19)
> (In reply to Andrey Loskutov from comment #17)
> 
> > If you look on the patch, there isn't much to be "fixed" except to revert
> > it. 
> 
> 
> Fixing the margins would IMHO be the right fix.

Please look on the GTK screenshots. Not margins is the biggest problem there, but border. They make the UI looks bad.
Comment 21 Lars Vogel CLA 2020-01-10 05:10:49 EST
(In reply to Andrey Loskutov from comment #20)
> (In reply to Lars Vogel from comment #19)
> > (In reply to Andrey Loskutov from comment #17)
> > 
> > > If you look on the patch, there isn't much to be "fixed" except to revert
> > > it. 
> > 
> > 
> > Fixing the margins would IMHO be the right fix.
> 
> Please look on the GTK screenshots. Not margins is the biggest problem
> there, but border. They make the UI looks bad.

Isn't that our SWT default? Do you see same issue in the Preference search field?
Comment 22 Alexander Kurtakov CLA 2020-02-17 03:55:47 EST
Retarget.