Move corner fold to top right in 24 icons
ClosedPublic

Authored by davidhurka on Mar 22 2020, 3:24 PM.

Details

Summary
  • Move corner fold to top right in 24 icons

Within these 24 icons:

  • Make 16px versions of 2 icons (dark and light)
  • Make 24px versions of 3 icons (dark and light)
  • Make 24px symlink versions of 3 icons (dark and light)

Maybe not all icons look better now. Then I can revert that icons.

Closes T12806

Test Plan

100% screenshot to compare:


Large screenshots to compare:

Montages:

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.
davidhurka created this revision.Mar 22 2020, 3:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 22 2020, 3:24 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
davidhurka requested review of this revision.Mar 22 2020, 3:24 PM
davidhurka retitled this revision from Move corner fold to top right in 24 icons, make 16px versions of 2 icons (dark and light), make 24px versions of 3 icons (dark and light), make 24px symlink versions of 30icons (dark and light) to Move corner fold to top right in 24 icons.Mar 22 2020, 3:28 PM
davidhurka edited the summary of this revision. (Show Details)
davidhurka edited the test plan for this revision. (Show Details)
davidhurka added a reviewer: VDG.
davidhurka edited the summary of this revision. (Show Details)
ngraham added a subscriber: ngraham.

Fantastic work! JFYI some of these icons have monochrome 32px versions that would need the same treatment for consistency's sake, or else the style changes with size:

I don't think we should change the copy icon. Other than that, this patch looks great.

Why do you think we shouldn't change the copy icon? Then it would be the only one not using the new style.

Why do you think we shouldn't change the copy icon? Then it would be the only one not using the new style.

Admittedly, my justification isn't rock solid. I just think it looks worse. It makes the top right awfully empty/unbalanced and I think changing it is more likely to bother people.

Looks fine to me in practice:

I understand the concerns. It looks unbalanced. But these icons also look unbalanced now: acrobat, filename-title-amarok.

By the way I changed the arrow of document-export to point outwards the document, not inwards. That often confused me in inkscape, where both icons are used next to each other.

document-edit-encrypt and -decrypt are swapped now. Maybe that could confuse users who are used to look at the corner fold.

And insert-page-break looses the metaphor of “a break in the page”, but I think that’s ok.

I wasn’t aware that there are still 32px monochrome icons. I will fix that. Didn’t we want to drop them?

davidhurka updated this revision to Diff 78305.Mar 23 2020, 6:47 PM
  • link 32px document-preview-archive and document-preview
  • Move corner fold to top for 6 32px icons
davidhurka added a comment.EditedMar 23 2020, 6:48 PM

There are 172 icons in 16px but not in 22px, and 348 icons in 22px but not in 16px. There are 1561 icons in 22px but not in 32px, and 9 icons in 32px but not in 22px. What are 12px and 32px good for?

I fixed the 6 32px icons which had the old style. 2 additional 32px icons already had the corner fold in the top: document-edit-sign, document-preview. I linked document-preview and document-preview-archive.

100% overview:

Large:

Eh, I'm not going to make a big fuss about the copy icon.

There are 172 icons in 16px but not in 22px, and 348 icons in 22px but not in 16px. There are 1561 icons in 22px but not in 32px, and 9 icons in 32px but not in 22px. What are 12px and 32px good for?

12px are probably used by a vector graphics program. I don't see them used in Inkscape though. I doubt Karbon uses it, but I didn't check.

32px is seen sometimes. You can choose to set your toolbar icon size to 32px. It can also be useful for mobile phone and TV UIs when the 22px icons are kind of blurry and 16px symbolic icons start looking way too chunky.

ndavis requested changes to this revision.Mar 25 2020, 4:09 AM
ndavis added inline comments.
icons/actions/32/document-preview.svg
2

This needs to be a relative symlink (ln -sr document-preview-archive.svg document-preview.svg)

This revision now requires changes to proceed.Mar 25 2020, 4:09 AM
davidhurka updated this revision to Diff 78459.Mar 25 2020, 3:23 PM
  • Fix symlinks: make relative
davidhurka marked an inline comment as done.Mar 25 2020, 3:28 PM
davidhurka added inline comments.
icons-dark/actions/22/acrobat.svg
2

I realize that acrobat.svg had a fixed color, which I mistakenly replaced by colorScheme-Negative. Shall I change that back?

ngraham added inline comments.Mar 25 2020, 5:12 PM
icons-dark/actions/22/acrobat.svg
2

Yeah probably. The color is intrinsic to their brand, not something we should re-color.

davidhurka updated this revision to Diff 78484.Mar 25 2020, 6:01 PM
  • Give **/acrobat.svg its fixed color back
davidhurka added inline comments.Mar 25 2020, 6:01 PM
icons-dark/actions/22/acrobat.svg
2

Actually the 16px version of acrobat.svg was recolored by colorScheme-Negative. I will set the color fixed anyway.

davidhurka marked 3 inline comments as done.Mar 25 2020, 6:01 PM
ndavis accepted this revision.Mar 25 2020, 11:28 PM
This revision is now accepted and ready to land.Mar 25 2020, 11:28 PM
ngraham accepted this revision.Mar 26 2020, 12:23 AM

Feel free to land this, @davidhurka! Very nice work.

I would need a developer account to land this. Same for D27983 I assume?

Oh, you don't have one yet? That's surprising to me! You've submitted a ton of great stuff. You should apply for one and list me as the sponsor. :)

https://community.kde.org/Infrastructure/Get_a_Developer_Account

davidhurka requested review of this revision.Mar 28 2020, 3:37 PM

One potentially important thing: See inline comment.

icons/actions/22/filename-title-amarok.svg
8

I realized that http://notmart.org/blog/2016/05/icon-colors/ and https://community.kde.org/Guidelines_and_HOWTOs/Icon_Workflow_Tips#Stylesheets contratict in the class names. One uses ColorScheme-Highlight, the other ColorScheme-ButtonFocus. breeze-icon-cleaner uses ColorScheme-ButtonFocus. In the repository I see several icons which use ColorScheme-Highlight, and only a few which use ColorScheme-ButtonFocus.

So which one is correct? I assume that https://community.kde.org/Guidelines_and_HOWTOs/Icon_Workflow_Tips#Stylesheets needs to be fixed.

You can land this now. We can address the discrepancy between highlight and button focus color usage elsewhere.

ndavis added inline comments.Mar 31 2020, 4:09 AM
icons/actions/22/filename-title-amarok.svg
8

Yeah, I'd rather use ButtonFocus, but it actually doesn't work outside of plasmashell, so use Highlight instead

davidhurka updated this revision to Diff 79272.Apr 4 2020, 9:50 AM

Rebase on an initial commit which points to D28203, not D28204.

  • link 32px document-preview-archive and document-preview
  • Move corner fold to top for 6 32px icons
  • Fix symlinks: make relative
  • Give **/acrobat.svg its fixed color back

I created revision D28204 accidentally when I tried to update this revision. That made my initial commit point to D28204 instead of D28203, so I couldn’t tell arc to land this revision. So I made git commit --amend on the initial commit to change the revision number, and rebased the other commits on the new commit. Phabricator UI tells me that the patch content stayed identical, so I land this now.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 4 2020, 9:55 AM
This revision was automatically updated to reflect the committed changes.
kossebau added a subscriber: kossebau.EditedApr 4 2020, 11:02 AM

Hi. Seems this broke the build somehow, please check: https://build.kde.org/view/Failing/job/Frameworks/job/breeze-icons/

Remember this is KF 5.69 branching WE, so needs fixing ASAP to not block the release. Do not shoot the messenger :)

davidre reopened this revision.Apr 4 2020, 11:07 AM
davidre added a subscriber: davidre.

This broke the build. Please note that tagging is today

davidre requested changes to this revision.Apr 4 2020, 11:08 AM
This revision now requires changes to proceed.Apr 4 2020, 11:08 AM

Seems @kossebau was faster ;)

Fixed it. I missed a few more symlinks that weren't relative in the review. I couldn't test it locally during review because arc can't download patches that convert a file into a symlink or vice versa.

@ndavis commited (excerpt):

diff --git a/icons-dark/actions/22/document-export-ocal.svg b/icons-dark/actions/22/document-export-ocal.svg
index 4bfca303..ace4fe41 120000
--- a/icons-dark/actions/22/document-export-ocal.svg
+++ b/icons-dark/actions/22/document-export-ocal.svg
@@ -1 +1 @@
-/home/david/kde/breeze-icons/icons-dark/actions/22/document-export.svg
\ No newline at end of file
+document-export.svg
\ No newline at end of file

Yep, apparently you found the problem faster than me. :) I thought I fixed all the symlinks, wasn’t the case... Thanks for fixing my mess!

Yay for quick fixing, thanks (in name of KF release interest group) :)

Do not forget to set this review request as Closed again if you are done here now (cannot tell myself).

davidre accepted this revision.Apr 4 2020, 12:00 PM
This revision is now accepted and ready to land.Apr 4 2020, 12:00 PM
davidre closed this revision.Apr 4 2020, 12:00 PM