Remove deprecated STOCK constant

MightyCreak / diffuse

Diffuse is a graphical tool for comparing and merging text files. It can retrieve files for comparison from Bazaar, CVS, Darcs, Git, Mercurial, Monotone, RCS, Subversion, and SVK repositories.

http://mightycreak.github.io/diffuse/

GNU General Public License v2.0

268 stars 45 forks source link

Remove deprecated STOCK constant #203

Closed oscfdezdz closed 1 year ago

MightyCreak commented 1 year ago

Thanks for your contribution!

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

oscfdezdz commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Marking as draft until I include the missing icons.

MightyCreak commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I just remember that @mjourdan suggested a revamp of the UI here: https://github.com/MightyCreak/diffuse/issues/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 commented 1 year ago

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.

MightyCreak commented 1 year ago

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