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

Remove deprecated STOCK constant #203

Merged
merged 4 commits into from
Apr 11, 2023
Merged

Remove deprecated STOCK constant #203

merged 4 commits into from
Apr 11, 2023

Conversation

oscfdezdz
Copy link
Contributor

No description provided.

@MightyCreak
Copy link
Owner

Thanks for your contribution!

Have you verified that the results are the same as before in the UI?

@oscfdezdz
Copy link
Contributor Author

Yes, although symbolic icons change depending on the GTK version of the machine, this is how it looks in a Flatpak build:

Captura desde 2023-04-08 17-18-02

The texts of the dialog buttons are the same.

@MightyCreak
Copy link
Owner

There's an issue with the first two icons on the top-left. I know it's some kind of weird code behind it that creates new icons based on an official GNOME icon.

Few questions:

  • Do you think it would be possible to use the symbolic version for these icons?
  • How about using non symbolic icons for the toolbar?

@oscfdezdz
Copy link
Contributor Author

Do you think it would be possible to use the symbolic version for these icons?

Yes, although it will be needed to edit some of the existing ones by hand and include them, adding their route in Meson.

How about using non symbolic icons for the toolbar?

I think GTK does not include fullcolor icons in its latest versions, we could use the ones from previous versions.

Adapting those icons to symbolic may be easier than including all the fullcolor icons. We also have to adapt "Merge From Left Then Right" and "Merge From Right Then Left" icons.

@oscfdezdz
Copy link
Contributor Author

Marking as draft until I include the missing icons.

@oscfdezdz oscfdezdz marked this pull request as draft April 8, 2023 16:10
@MightyCreak
Copy link
Owner

Yes, although it will be needed to edit some of the existing ones by hand and include them, adding their route in Meson.

Would it be possible to use the same technique, but with symbolic icons?

Right now, it creates new icons based on the 'document-new' icon: https://github.com/MightyCreak/diffuse/blob/main/src/diffuse/window.py#L733

For merge left/right, it uses 'go-next' and 'go-previous'

@oscfdezdz
Copy link
Contributor Author

oscfdezdz commented Apr 8, 2023

Would it be possible to use the same technique, but with symbolic icons?

Oh, yes, I meant symbolic icons, sorry for the confusion.

Right now, it creates new icons based on the 'document-new' icon: https://github.com/MightyCreak/diffuse/blob/main/src/diffuse/window.py#L733

For merge left/right, it uses 'go-next' and 'go-previous'

Reusing that code for merge left/right doesn't look bad:

imagen

However, I've managed to tweak document-new-symbolic and horizontal-arrows-symbolic from Icon Library to get symbolic icons for those:

imagen

What do you think?

This screenshot is with SMALL_TOOLBAR icon size which I think, looks better:
imagen

@oscfdezdz oscfdezdz marked this pull request as ready for review April 8, 2023 23:58
@oscfdezdz
Copy link
Contributor Author

I think GTK does not include fullcolor icons in its latest versions, we could use the ones from previous versions.

Sorry, this is totally wrong. Of course it includes fullcolor icons, it's what is currently using and Flatpak build with latest runtime which includes latest GTK3 version has them. Although they look a bit dated.

@MightyCreak
Copy link
Owner

MightyCreak commented Apr 11, 2023

I think symbolic icons are better than the fullcolor ones. And I think the best version is the second one you proposed (i.e. custom icons, large toolbar):

Here are my reasons why:

  • I like the new 2- and 3-way merge icons you did in the 2nd screenshot, it's clearer than the ones in the first screenshot (I was indeed expecting overlapping as symbolic icons are transparent in the middle)
  • I like the icons you made based on the 'horizontal-arrows-symbolic' icon in the 2nd screenshot, it also allows to differentiate the 4 previous icons about "copying" from these 2 that are about "merging"
  • I think the large toolbar (in the 2nd screenshot) works better than the small toolbar (in the 3rd screenshot) because I feel that when you see two rows of small icons, it makes it more difficult to separate the window toolbar from the panels toolbars

What do you think?

@MightyCreak
Copy link
Owner

I just remember that @mjourdan suggested a revamp of the UI here: #90

Take a look at the icons he used for the merge menu. I just don't know if these are stock icons or where to get them.

@oscfdezdz
Copy link
Contributor Author

oscfdezdz commented Apr 11, 2023

  • I think the large toolbar (in the 2nd screenshot) works better than the small toolbar (in the 3rd screenshot) because I feel that when you see two rows of small icons, it makes it more difficult to separate the window toolbar from the panels toolbars

It makes sense, leaving the large toolbar.

I just remember that @mjourdan suggested a revamp of the UI here: #90

Take a look at the icons he used for the merge menu. I just don't know if these are stock icons or where to get them.

I ended up using the suggested icons for the merge menu. The icons are step-over-symbolic, step-back-symbolic, mail-reply-all-rtl-symbolic, mail-reply-all-symbolic, document-revert-rtl-symbolic and document-revert-symbolic. The first two are not included but are available on Icon Library, and the others are, although I have modified reply-mail-all icons so that they are vertically mirrored compared to the original stock icons to match the mockups.

image

Using gresource file and based on the suggestions for the merge menu
made in #90.
Now that custom icons are bundled in the build, this code is no longer
used.
@MightyCreak
Copy link
Owner

Thank you so much for your effort! This is so nice! And on top of that it reduces the number of lines of code 🤩

@MightyCreak MightyCreak merged commit a28f194 into MightyCreak:main Apr 11, 2023
3 checks passed
@oscfdezdz oscfdezdz deleted the deprecated branch April 12, 2023 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants