-
-
Notifications
You must be signed in to change notification settings - Fork 60
Fix/progess spinner resizing #238
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
base: main
Are you sure you want to change the base?
Conversation
|
My concern with resizable spinners is that I'll often do something like HStack {
Spacer()
ProgressView()
Spacer()
}to center a spinner within a larger view. Would this lead to a ridiculously large spinner on a large enough screen? |
Perhaps this resizability can be gated behind a |
Not resizable by default, can be changed at runtime. Does not affect ProgressBarView
Exactly what I did. I’m going to replace the function with how its done on the Image in a moment. I like it better. |
|
It doesn't need to be done now (I could do this in a future PR), but maybe some code could be added to UIKitBackend to change the spinner between Whatever threshold we choose, we have to make sure it's large enough that the |
totally. I decided to not do it in this pr because the scope was scui isolated and I wanted to stay it that way. (Also I wasn't sure yet how I'd like the api to look for it. Single Backend support is way harder for api design imo. Especially when its kind of overlapping with another modifier but not really.) Maybe .style() or whatever it would be called could apply .resizable() and .frame(constant size) depending on .medium or .large? so it wouldn't be UIKit Backend specific anymore (even though the other backends wouldn't need specific code) |
stackotter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Just a few requested changes. Other than those, I'll test locally when I get the time and then I'll be happy to merge.
|
Here's what I've found from testing locally; GtkBackend and Gtk3Backend
AppKitBackend
UIKitBackend
WinUIBackend
|
…nner sized pre view graph insertion Doesn't seem to be a performance issue, but its not a nice way. Sadly nothing else I tried works.
stackotter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small changes requested. Mostly just stylistic things though, the implementation looks great now!
| Button("Randomize progress view size") { | ||
| progressViewSize = Int.random(in: 10...100) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be removed. I assume it was probably just used to test for size animation when fixing the AppKitBackend issues?
| /// Required due to AppKitBackend needing special treatment. | ||
| /// Forward to ``AppBackend/setSize(of widget: Widget, to size: SIMD2<Int>)`` | ||
| /// on other Backends. | ||
| func setProgressSpinnerSize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to setSize(ofProgressSpinner:to:) for consistency.
| /// Changes the Spinner's Size. | ||
| /// Required due to AppKitBackend needing special treatment. | ||
| /// Forward to ``AppBackend/setSize(of widget: Widget, to size: SIMD2<Int>)`` | ||
| /// on other Backends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace these docs with the following.
/// Sets the size of a progress spinner.
///
/// This method exists because AppKitBackend requires special handling to resize progress spinners.
///
/// The default implementation forwards to ``AppBackend/setSize(of:to:)``.I know that most of the other backend methods are just one big blob of text, but I'm planning on changing that soon, and I figured that given that I was already suggesting wording changes and fixing the symbol link, I might as well update it to the new formatting while I was at it.
Putting the blank lines splits the documentation into multiple paragraphs. The first paragraph is the one that LSPs show, and it's the short blurb used in generated DocC documentation under symbol links I think.
Fixes #237
Visually Breaking Change for some existing UIs using ProgressSpinner
This PR makes ProgressSpinners resizable on supported backends. Unsupported Backends change the container size without changing how the Widgets is actually being rendered to the user.
All changes are SCUI-Level, so even Backends I didn’t test (Gtk3) should benefit from it without breaking.
It may break some existing UIs though, as it now can be squished smaller than its natural size. To make it not entirely disappear (without setting a frame manually) I set its width/height to a minimum of 10 points, it can‘t shrink smaller.
To determine its rendered size it takes the smaller axis length and applies it to both width and height (making sure it always fits inside the applied constraints).
This can lead to it being rendered smaller than a set frame, if only one axis is set and its being squished from the opposite site. setting both axes in .frame eliminates this „problem“.
Gtk seems to have a min size for its Spinner. This can lead to clipping on too small externally applied constraints (at least on Ubuntu). I don’t really consider this to be an issue though as it can easily be avoided by the developer and other views can clip too.