Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Action Icons Redesign #1735

Closed
wants to merge 19 commits into from
Closed

Conversation

Fatih20
Copy link
Contributor

@Fatih20 Fatih20 commented Feb 7, 2020

I'm a user of this cool app on Linux and upon using it I found that the icons here are very unpleasant. I'm no professional by any means but I have spent the last few months contributing icons for other open source projects.

This is certainly not my final proposal, I wanted to listen to feedback first before continuing the redesign of all the action icon. There are some icon that haven't been redesigned because of that.

Feel free to ask me why I made certain icon the way they are if you'd like to gain some insight.

@Rmano
Copy link
Contributor

Rmano commented Feb 7, 2020

It would be nice to have a screenshot of the new icons here... ;-)

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 7, 2020

I haven't taken times to compile the software, I'll get on it later.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 7, 2020

Screenshot from 2020-02-07 21 30 23
Screenshot from 2020-02-07 21 29 48
Screenshot from 2020-02-07 21 29 15

@Technius
Copy link
Member

Technius commented Feb 8, 2020

Interesting work! A few comments:

  • In my opinion, the eraser looks like a magnet.
  • The latex icon is an upgrade, but it might look better in a serif font.
  • Not sure if this was intentional, but all of the file permissions were changed.
  • The new Default tool icon looks like an update icon. Not sure how to compare with the current one though, since the current one looks pretty confusing (in my opinion).
  • The redesigned Hand Tool icon is a pointer, but that suggests selection rather than panning.
  • How well the icons look is going to depend a lot on the icon theme. For example, I use the Papyrus icon theme and I think the current minimalist look goes really well with it, but the redesign clashes in style.
  • I just realized that we don't need custom icons for the audio player, since the XDG desktop icon specification has the media-* icons.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 8, 2020

If XDG already has it, why does the current version use its own icon?
Yes, I agree the current one is shit and probably is just taken from an obscure free stock vector website.
I'd also like to add that while the current version is minimalist like Papyrus, it is far from the quality of Papyrus.
I'll look into your other concern regarding the individual icon.

@nicegamer7
Copy link

I have some minor notes:

  • When I think of a highlighter, I usually think yellow
  • The hand tool is more red than I'd expect

I really like them! Good job.

@Rmano
Copy link
Contributor

Rmano commented Feb 8, 2020

Definitely a nice step forward, I like them.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 8, 2020

Cool. What would it takes for this to be merged?

@Technius
Copy link
Member

Sorry for the late response, I'll take another look at this when I have time tomorrow.

@Technius
Copy link
Member

I was very busy this week, but I finally had the time to review today.

  • Please revert the file permissions on the icons to 644.
  • The rotation snapping icon doesn't look right; the rotation icon blends in too much with the magnet. This will be problematic to people who have some color blindness.
  • The grid snapping icon isn't descriptive enough--there needs to be some indicator that it concerns the grid.
  • Why was the color of the icons in the sidebar changed?
  • The new presentation mode icon is very nice.
  • The various draw mode tools (e.g. arrow, line, etc.) have too much line thickness. For example, it is hard to see the endpoints of the Draw Line icon or the arrowhead of the Draw Arrow icon.
  • Vertical Space icon looks too much like a select icon, but it does something much different.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 23, 2020

Regarding the grid snapping, the reason I didn't make it about grid is because it should've been named just "snapping" since it works without the grid being shown/activated. By the way, the rotation snapping icon should've been renamed from "snapping" to "rotation snapping", because it only applies rotation snapping.
The color of the sidebar icon is now red because I'd like to give similar theme to icons that are placed in smilar location and has function concerning the same thing. Red is just a good color because it can do the various shading and whatnot that I wanted to do on the sidebar icon.
Vertical space icon looked like the select icon because in the current version, it also looked a lot like the select icon.

I'll look into your other criticism and fix it.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 2, 2020

image
Changes as promised. @Technius

@LittleHuba
Copy link
Member

Adding a new page and creating a new document look too similar. I would change the create a document icon to a page with a yellow or golden star (*). That would remove the possibility of confusing both. Additionally the star is normally the sign used for these types of icons where you create a new file.

@LittleHuba
Copy link
Member

Also all the different colors make the design extremely unrestful and (sorry to phrase it that way) childlike. I'd stick to a small pallet of colors and make the icons mostly monochrome. This also simplifies compatibility with different themes users might choose.

The icons themselves look amazing though! I like the new rotation snapping!

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 3, 2020

Thanks for the compliment!
Creating a new document and adding page icon isn't part of this redesign. Creating a new document icon is covered by the XDG desktop icon specification not from the xournalpp, I might redesign adding page to differentiate it though.

I could design 2 set of icon, one monochrome and one colourful, and then the user themselves could choose which icon they'd prefer. But that requires a coder putting the options in the preferences.

Maybe a poll like the one for infinite pages if I could only design one? I don't know tho, it's up to the maintainer.

@nicegamer7
Copy link

Personally I like the coloured icons, so an option to pick between coloured and monochrome would be preferable.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 17, 2020

Any further inputs before merging this? @Technius

@Technius
Copy link
Member

This is mostly ready for merge, but I have to agree with @nicegamer7 in that having a selectable colored/monochrome theme would be nice. Here's a simple plan to do this:

  1. You'll have to put the new (colored) icons in a different folder, e.g. ui/iconsNew.
  2. We'll merge in the changes into a separate branch.
  3. From there, we'll add code in the settings that will allow users to select between the colored icons and the monochrome icons.
  4. Once we have done so, we'll merge the new icon branch back into master.

Does this sound good? @LittleHuba @Fatih20

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 20, 2020

Sounds like it, but I prefer to get rid of the old icon entirely. I could design the monochrome icon that you asked for.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 25, 2020

image
Alright so here's how it's going to be. Monochrome in IconsMono and Colorful in IconsColor. Now, there's this IconsDark that IMO is unnecessary since the colorful icon works with both light and dark mode and the monochrome icon I'm making will follow. I'm going to get rid of it since it seems to be obsolete unless you think otherwise. @Technius

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 25, 2020

image
This is the first draft of the monochrome icons, any opinions? @Technius @LittleHuba
This is the color palette I use.
image

@Technius Technius mentioned this pull request Mar 25, 2020
@Technius
Copy link
Member

That looks great! Just one minor issue: the Fine stroke is just a black dot, which may not be visible on dark themes (this is how it looks currently, but I think we should improve it if we can).

Now, there's this IconsDark that IMO is unnecessary since the colorful icon works with both light and dark mode and the monochrome icon I'm making will follow. I'm going to get rid of it since it seems to be obsolete unless you think otherwise.

Yeah, we probably won't need iconsDark anymore.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Mar 26, 2020

Just curious, what year was it when the original icon got implemented?

@Fatih20
Copy link
Contributor Author

Fatih20 commented Dec 28, 2020

@Fatih20 Having your new icons merged would be a great plus when releasing version 1.1.0, since it will look nice and fresh. Do you need help finishing this PR?

The icon is done. My part here is finished.

From my understanding, the rest now lie on @LittleHuba and others to make a UI to switch icons between monochrome and colored, among other things.

@rolandlo
Copy link
Member

@LittleHuba @Technius How do we proceed from here? Do you need some help? @Fatih20 has completed his part, so it is on us to finish this off.

@LittleHuba
Copy link
Member

@Febbe is the new application initialization finished? That was the blocker for this PR.

I already tried some ideas on how to enable switching of themes. We can't use the current system as it only supports the light and dark version. I haven't got it running so far.

We also need to revisit the PR for the new application icon. I had to revert it as Windows releases did not show any icon at all after merging it. I suspect a missing file there. @rolandlo would you mind checking this out?

@rolandlo
Copy link
Member

@LittleHuba Yes, you reverted it, but then a new version of the PR got merged, see #2557. I can check if it works on Windows.

@LittleHuba
Copy link
Member

Then it should be good. People would have complained by now.

@rolandlo
Copy link
Member

I have checked it nonetheless. Xournal++ (latest nightly release) works with the new icons. The new icon is correctly displayed inside the app (as seen in the About dialog). It is also correctly displayed in the taskbar while running. The icon displayed in the Windows Start menu was the old one, however. It is also used if you right click the executable and attach to the taskbar.

As I have noticed, the xournalpp.ico file (in the windows-setup folder) has not been updated. It still shows the old icon. We must fix that. I guess we simply convert the svg-file of the icon into .ico format using a tool like convertfrom ImageMagick.

@LittleHuba
Copy link
Member

Nice catch @rolandlo. Could you modify the build script to do this for us automatically? This might be the best approach as long as we can use some pre-existing or easy-to-install dependency for this. I would like to remove the redundancy for this situation.

@rolandlo
Copy link
Member

@LittleHuba Ok, I will modify the build scripts, also accounting for the missing plugin and ressource folders. Just give me some time.

@rolandlo rolandlo modified the milestones: v1.1.0, v1.1.0 release Jan 21, 2021
@Technius Technius linked an issue Jan 22, 2021 that may be closed by this pull request
@Technius
Copy link
Member

Technius commented Feb 3, 2021

Hi @Fatih20, can you clarify what license you are contributing these icons under? The code must be under GPLv2, but the icons are your original contribution. Based on the image metadata, it seems to be some variant of Creative Commons? Need to know for #2148.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 17, 2021

Hi @Fatih20, can you clarify what license you are contributing these icons under? The code must be under GPLv2, but the icons are your original contribution. Based on the image metadata, it seems to be some variant of Creative Commons? Need to know for #2148.

Yes I'm contributing it under GPLv2. Do I need to change the metadata?

@Technius
Copy link
Member

Do I need to change the metadata?

Please do so if it's easy. If it's too troublesome, we can add the metadata info for you.

@Fatih20
Copy link
Contributor Author

Fatih20 commented Feb 17, 2021

Do I need to change the metadata?

Please do so if it's easy. If it's too troublesome, we can add the metadata info for you.

You can add them yourself. Thanks.

@rolandlo
Copy link
Member

@LittleHuba Any progress on enabling theme switching?

@rolandlo rolandlo mentioned this pull request Feb 19, 2021
@Technius
Copy link
Member

Technius commented Feb 20, 2021

@rolandlo I can implement the theme switching stuff if no one else has time for it. For now, I think we should merge this into the new-icons branch and deal with the code in a follow-up PR. (need to resolve the merge conflicts by using the new icons though).

@rolandlo
Copy link
Member

@rolandlo I can implement the theme switching stuff if no one else has time for it. For now, I think we should merge this into the new-icons branch and deal with the code in a follow-up PR. (need to resolve the merge conflicts by using the new icons though).

I would appreciate that.

@mjg
Copy link
Contributor

mjg commented Feb 21, 2021

Just a note on the relation to #2795: Over there I suggested similar icon renames but dropped them now. I do introduce a new icon sidebar-layerstack (named to match the new name sidebar-layer here) copied from the existing (legacy) layer icon with an added arrow. The layerstack sidebar view displays the layer stack progressively (up to a certain layer) to match the new export option. It would be great if you introduce a stacked/progressive version of your new sidebar-layer icon, too, and name it sidebar-layerstack.

@LittleHuba
Copy link
Member

@rolandlo I can implement the theme switching stuff if no one else has time for it. For now, I think we should merge this into the new-icons branch and deal with the code in a follow-up PR. (need to resolve the merge conflicts by using the new icons though).

@Technius please do take over.

@Technius Technius closed this Feb 28, 2021
@Technius Technius deleted the branch xournalpp:new-icons February 28, 2021 00:01
@Technius
Copy link
Member

Oops, let me see if I can cherry pick the commits on to another branch... It got out of sync and became hard to merge.

@Technius Technius mentioned this pull request Feb 28, 2021
4 tasks
@Technius
Copy link
Member

See #2886 for follow up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

To many different icon styles Icon for new page and new document are too similar
10 participants