Differential D17419

Add view-private icon
ClosedPublic
Actions

Authored by GB_2 on Dec 7 2018, 10:09 PM.

Details

Reviewers
ngraham
ndavis
Group Reviewers
Breeze
VDG
Falkon
Commits
R266:b849a63cefb3: Add view-private icon
Summary

BUG: 401646

Test Plan

Search the icon in Cuttlefish.

Diff Detail

Repository
R266 Breeze Icons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
GB_2 created this revision.Dec 7 2018, 10:09 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 7 2018, 10:09 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
GB_2 requested review of this revision.Dec 7 2018, 10:09 PM
ngraham added a subscriber: ngraham.Dec 7 2018, 10:14 PM

I like it! What do you think about making the bottom line of the hat a bit wider, so it's more clear that it's a hat?

GB_2 updated this revision to Diff 47081.Dec 7 2018, 11:05 PM
GB_2 edited the summary of this revision. (Show Details)

Improve icons.

filipf added a subscriber: filipf.EditedDec 7 2018, 11:36 PM

Very Chrome/-ium like :)

Maybe expanding the bottom line just a tad more would make it more obvious it's a private eye's hat? Right now it still looks a bit more like eyebrows to me.

Firefox's mask also maybe isn't a bad idea:

ngraham accepted this revision.Dec 7 2018, 11:41 PM
ngraham added reviewers: VDG, Falkon.

Looks great to me! Thanks for embedding the CSS stylesheet too. Falkon or other VDG folks, any comments?

This revision is now accepted and ready to land.Dec 7 2018, 11:42 PM
GB_2 added a comment.EditedDec 8 2018, 12:22 AM
In D17419#372992, @filipf wrote:

Very Chrome/-ium like :)

Maybe expanding the bottom line just a tad more would make it more obvious it's a private eye's hat? Right now it still looks a bit more like eyebrows to me.

Firefox's mask also maybe isn't a bad idea:

Sure, I'll improve it!
I'll make the bottom line longer and the hat a bit smaller and higher.

ndavis added a subscriber: ndavis.Dec 8 2018, 3:12 AM

I would like if it was more aligned with the grid:

See how the top version is a bit more crisp?


GB_2 added a comment.Dec 8 2018, 10:12 AM
This comment was removed by GB_2.
GB_2 updated this revision to Diff 47093.Dec 8 2018, 10:18 AM
GB_2 edited the summary of this revision. (Show Details)

Improve icons.

GB_2 updated this revision to Diff 47106.Dec 8 2018, 1:21 PM
GB_2 edited the summary of this revision. (Show Details)
GB_2 edited the test plan for this revision. (Show Details)

Improve icons.

ngraham added a comment.Dec 8 2018, 2:26 PM

Hmm, now it seems like his hat gets flat at the 48px size:

GB_2 added a comment.Dec 8 2018, 2:40 PM

How is that even possible? There is no 48 px size icon and the one you showed was and older version of the icon...

ngraham added a comment.Dec 8 2018, 2:46 PM

Oh, probably a Cuttlefish bug then.

GB_2 added a comment.Dec 8 2018, 2:48 PM

Oh, thanks for reminding me about Cuttlefish...
I forgot about it, but now I don't need a QML for testing icons anymore :P

GB_2 edited the test plan for this revision. (Show Details)Dec 8 2018, 2:49 PM
ndavis requested changes to this revision.EditedDec 8 2018, 6:09 PM

I noticed that the 24px version is now aligned to the grid, but the margins aren't correct. The 16 and 22px versions are still not aligned to the grid.

The correct margin size for 24px is 4px on each side, but left and right margins can be exceeded if necessary. The 24px size is really only for applications that require 24px icons, such as some GTK applications. Do you actually need to use this size? There is no issue with adding it as long as it's correct, but it is slightly more work for you.

This revision now requires changes to proceed.Dec 8 2018, 6:09 PM
GB_2 updated this revision to Diff 47143.Dec 8 2018, 6:53 PM
GB_2 edited the summary of this revision. (Show Details)

Only provide correct 16px icons.

GB_2 updated this revision to Diff 47145.Dec 8 2018, 7:51 PM
GB_2 retitled this revision from Add private-mode icon to Add view-private icon.

Change name to the more common "view-private" and add a symlink for the symbolic icon.

GB_2 edited the summary of this revision. (Show Details)Dec 8 2018, 7:59 PM
ndavis accepted this revision.Dec 8 2018, 8:01 PM

Looks right. I might prefer a wider hat brim or a mask like @filipf suggested, but I'm not going to make that a requirement since it seems more like a matter of taste.

Here's roughly how it will look in Falkon:

Here it is with a wider brim on the hat:

This revision is now accepted and ready to land.Dec 8 2018, 8:01 PM
GB_2 added a comment.Dec 8 2018, 8:20 PM
This comment was removed by GB_2.
GB_2 added a comment.Dec 8 2018, 8:26 PM
This comment was removed by GB_2.
GB_2 updated this revision to Diff 47147.Dec 8 2018, 9:02 PM
GB_2 edited the summary of this revision. (Show Details)

Improve icons (wider hat brim).
It's perfect now :-)

ngraham added a comment.Dec 8 2018, 9:08 PM

If view-private-symbolic is just a symlink to the regular icon, do we really need it?

GB_2 added a comment.Dec 8 2018, 9:16 PM
In D17419#373590, @ngraham wrote:

If view-private-symbolic is just a symlink to the regular icon, do we really need it?

It's for compatibility, because other icons in Breeze and also other icon themes do it.

ndavis added a comment.Dec 8 2018, 9:17 PM
In D17419#373590, @ngraham wrote:

If view-private-symbolic is just a symlink to the regular icon, do we really need it?

I think the whole *-symbolic thing is a GNOME-ism. It's their way of specifying whether to use a color icon or a monochrome icon.

ngraham accepted this revision.Dec 9 2018, 6:49 PM

Ah yeah, I guess that makes sense.

Falkon folks, is this acceptable?

drosca added a subscriber: drosca.Dec 9 2018, 6:56 PM
In D17419#374064, @ngraham wrote:

Ah yeah, I guess that makes sense.

Falkon folks, is this acceptable?

Yes, looks great!

Closed by commit R266:b849a63cefb3: Add view-private icon (authored by GB_2, committed by ngraham). · Explain WhyDec 9 2018, 7:01 PM
This revision was automatically updated to reflect the committed changes.
ndavis added a comment.Dec 10 2018, 11:28 AM
This comment was removed by ndavis.

Revision Contents
Changeset List

Diff 47205

View Options

icons-dark/actions/16/view-private.svg

Loading...
View Options

icons-dark/actions/symbolic/view-private-symbolic.svg

Loading...
View Options

icons/actions/16/view-private.svg

Loading...
View Options

icons/actions/symbolic/view-private-symbolic.svg

Loading...