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

GTK4 port #138

Merged
merged 38 commits into from
Mar 23, 2023
Merged

GTK4 port #138

merged 38 commits into from
Mar 23, 2023

Conversation

Huluti
Copy link
Owner

@Huluti Huluti commented Mar 20, 2023

Fix #88.

@Huluti Huluti added the tech/UI label Mar 20, 2023
@Huluti Huluti changed the title Initial GTK4 port Initial GTK4 port (#88) Mar 20, 2023
@Huluti Huluti changed the title Initial GTK4 port (#88) Initial GTK4 port Mar 20, 2023
@Huluti Huluti mentioned this pull request Mar 20, 2023
@Huluti Huluti changed the title Initial GTK4 port GTK4 port Mar 20, 2023
@Huluti Huluti added this to the 1.4.0 milestone Mar 20, 2023
src/ui/apply.ui Outdated Show resolved Hide resolved
src/ui/apply.ui Outdated Show resolved Hide resolved
src/ui/apply.ui Outdated Show resolved Hide resolved
src/ui/apply.ui Outdated Show resolved Hide resolved
src/ui/window.ui Outdated Show resolved Hide resolved
src/window.py Outdated Show resolved Hide resolved
src/window.py Outdated Show resolved Hide resolved
com.github.huluti.Curtail.json Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
src/window.py Outdated Show resolved Hide resolved
@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 20, 2023

This is more of a general review of the app than a review of the port itself. Some items might fall outside of the scope of the MR, please feel free to ignore them or leave them for future MRs.

@bertob
Copy link
Contributor

bertob commented Mar 20, 2023

Is it just me or is it not using libadwaita styles? This is what the branch looks like here:

image

@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 20, 2023

No, its not using libadwaita, Ill mention it again. As a circle GTK 4 app using libadwaita is a requirement.

@Huluti
Copy link
Owner Author

Huluti commented Mar 21, 2023

@bertob it's coming! ;) just pushed a new commit. And I have started to pick some ideas of #119.

image

thanks @A6GibKm for this big review, I'll try to fix all your points.

@Huluti
Copy link
Owner Author

Huluti commented Mar 21, 2023

I'll try to use more Adwaita widgets like StatusPage and AdwAboutWindow.

src/window.py Outdated Show resolved Hide resolved
src/window.py Outdated Show resolved Hide resolved
src/window.py Show resolved Hide resolved
src/ui/window.ui Outdated
<property name="visible">True</property>
<property name="can-focus">True</property>
<property name="receives-default">True</property>
<property name="label" translatable="yes">Browse files</property>
Copy link
Contributor

@A6GibKm A6GibKm Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Browse Files, all buttons use Header Capitalization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tooltip texts also use Header Capitalization

@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 21, 2023

@bertob it's coming! ;) just pushed a new commit. And I have started to pick some ideas of #119.
image
thanks @A6GibKm for this big review, I'll try to fix all your points.

Probably the pill button shouldn't be expanded horizontally, pill by itself already gives it sufficient emphasis

Don't find how to not expand it...

The parent box has hexpand set to True, maybe that is causing it.

@Huluti
Copy link
Owner Author

Huluti commented Mar 21, 2023

@bertob latest version:
image

@bertob
Copy link
Contributor

bertob commented Mar 21, 2023

Tried the latest version, it's using the correct styles now! I noticed that the preferences dialog doesn't use the Adwaita preferences window widget, would be good for consistency:

image

@Huluti
Copy link
Owner Author

Huluti commented Mar 22, 2023

@bertob done :)

image

src/ui/preferences.ui Outdated Show resolved Hide resolved
src/ui/preferences.ui Outdated Show resolved Hide resolved
src/ui/preferences.ui Outdated Show resolved Hide resolved
@bertob
Copy link
Contributor

bertob commented Mar 22, 2023

A few more things I noticed:

  • The view with the file list is a treeview, and feels cramped and not very legible. Given the amount of information we're displaying here (just filename + 3 pieces of metadata) I think a listbox would be a much better fit here, maybe with file name as the title, the two different sizes as subtitle, and the savings in green/red on the right side of the row. Switching to list rows would also have the advantage that the rows would be taller, which would allow for image thumbnails, which would really help to know which image is which at a glance.
  • It's not clear to me what exactly the back/forward buttons are for. They seem to just go back to the empty state after you've converted some files? If so, I don't think this is the best pattern for this, I'd go with something like a "clear" button instead (e.g. in the primary menu).
  • There's a "browse" button on the welcome view but not after that. I'd have an open button in the top left, or a floating pill button in the bottom center for that.
  • The layout of the welcome view feels a bit messy, there are lots of elements with different sizes and shapes. Some suggestions:
    • Use a standard status page layout with the app icon at the top
    • Move the open button to the top left, or make it a floating pill button
    • The lossless/lossy switch is a bit odd, and IMO the options could use a bit more context. Perhaps two radio button rows would work here, then we could also have a subtitle for each to explain the options?
    • "Images are saved with -min prefix" could perhaps be integrated in the description

@Huluti
Copy link
Owner Author

Huluti commented Mar 22, 2023

I'll do a release with this PR. Like that we now have a Gtk 4 version of Curtail.

And will check your new (great) points in other PRs.

Thank you very much to both of you!!

@Huluti Huluti merged commit cd56d88 into master Mar 23, 2023
@Huluti Huluti deleted the gtk4-port branch March 23, 2023 11:28
@Huluti
Copy link
Owner Author

Huluti commented Mar 24, 2023

image

@bertob are we on something? (for the listbox) 😎 I'm just trying to have larger previews here but it doesn't seem possible.

@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 25, 2023

How are you adding them? Its certainly possible to have then at the same size of the row. Maybe you need a custom widget there depending on the style you want though

@bertob
Copy link
Contributor

bertob commented Mar 26, 2023

@Huluti Yeah, looks good! One thing that would be nice to avoid is having that huge bar with the "Images are saved as..." label at the bottom. Perhaps that could be a subtitle in the headerbar?

@Huluti
Copy link
Owner Author

Huluti commented Mar 26, 2023

Yes I will move that! I'm doing things step by step :) Keep you posted.

@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 26, 2023

image

@bertob are we on something? (for the listbox) 😎 I'm just trying to have larger previews here but it doesn't seem possible.

From the photo i noticed the list box is being cut by the message. As a first approach one should add a gtk separator in between, but this might be a good use case for a adw banner. I would ask in the adwaita channel to be sure

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

Successfully merging this pull request may close these issues.

Port to GTK4
3 participants