-
-
Notifications
You must be signed in to change notification settings - Fork 687
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
Add OptionContainer for iOS #2259
Conversation
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.
iOS has very specific behaviour around its "more" tab; it's not clear if that behavior will be replicated on Android
I don't think the equivalent Android API has any built-in "more" behavior, but it should be possible to replicate this behavior with a little additional code – perhaps using a dialog rather than a list in the tab itself.
I've added a presumptive branch that would cover the alternate possible behavior
Which branch do you mean?
to display for that title; or 3-tuples, describing the title, content widget, | ||
and icon for the content. |
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.
Threre's an inconsistency between the constructor, which accepts (text, widget, icon)
, and the append
and insert
methods, which accept (text, widget, enabled, icon)
.
I suggest limiting the constructor to just the required arguments (text, widget)
. Then enabled
and icon
can be set either by assigning the properties on the OptionItem
, or omitting the content in the constructor entirely and using append
.
It would also be a good idea if the enabled
and icon
arguments of append
and insert
were made keyword-only, since the meaning of bare True
and False
arguments wouldn't be obvious in this context.
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.
Agreed it is inconsistent. My inclination for a consistency fix would be slightly different, though:
- Change the constructor to
(text, widget, icon, enabled)
- Change
insert()
to be(text, widget, *, icon, enabled)
This is on the basis that icon is more likely to be used at time of construction than enabled (since it's in-practice required for use on mobile); neither argument was previously exposed at time of construction; and if the argument is being made keyword-only on insert()
, there's no historical order interpretation to consider (although there's a backwards-incompatible change that will be caught by the change to being keyword-only).
The other approach that I contemplated was introducing a namedtuple that declares the actual arguments, so you could easily define a initial content of content=[Option("foo", toga.Box(), enabled=False)]
. Not sure if that would be part of the global Toga namespace, (toga.OptionContainer(content=[toga.Option("foo", toga.Box(), enabled=False)]
), or stored on the OptionContainer itself ((toga.OptionContainer(content=[toga.OptionContainer.Option("foo", toga.Box(), enabled=False)]
).
Of course, it also raises the question of what is different between an OptionItem
and this new namedtuple. I guess the ideal situation would be to allow the user to pass in OptionItem
objects, but that will require a bit of surgery on the rest of the interface. It also raises the question of what to do with the insert()
method - do we allow insert(n, OptionItem(...))
?
This larger OptionItem change could be deferred to a later PR, though. Even if we do go down this path, we'd still need to continue to support the basic tuple on construction.
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.
- Change the constructor to
(text, widget, icon, enabled)
- Change
insert()
to be(text, widget, *, icon, enabled)
The problem with that is what it does to the constructor documentation:
And if we add OptionItem as an alternative later, it'll get even worse.
So how about this:
- Give OptionItem a constructor with arguments
(text, widget, *, icon, enabled)
- Make the OptionContainer constructor,
insert
andappend
all accept OptionItem. It looks like the alternative signatures ofinsert
andappend
could be documented using@overload
, though we could also consider deprecating the non-OptionItem versions.
This would also be extensible to any more attributes we add to OptionItem in the future.
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.
As with the Image constructor typing, this is one of those areas where my argument is that the problem is with the typing process, not with the actual interface. The actual interface isn't that complicated in practice. What is unwieldy is the syntax for formally describing that interface.
As I flagged in my last comment: using OptionItem will require a bit of surgery. The interface on OptionItem
is the issue; it's not currently set up to be user-exposed. However, I'll take a look to see what can be done.
Co-authored-by: Malcolm Smith <smith@chaquo.com>
FWIW, for a first pass, I'd also be entirely happy with a "can't support more than 5 tabs" implementation on Android. That was my original plan with iOS, until the "more" tab literally fell out with almost no code. I spent longer working out how to test it than I did implementing it.
I'm referring to the "else" branch when testing "more_option_is_stateful". At preset, iOS is the only platform with a more option, and it's always stateful. If Android is also stateful, then the |
Ok - I've been able to make the Rendering the overloads in Sphinx, though... that's an absolute clusterf*ck. What is currently committed is as good as I can make it look. Which isn't good. The |
Another update, including "C" type definitions for the IconContent and OptionContainerContent types, plus references in documentation for those types. |
core/src/toga/icons.py
Outdated
if TYPE_CHECKING: | ||
from typing import TypeAlias |
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.
As a note, we can avoid this by depending on the typing_extensions
package; it backports all the new stuff for typing.
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.
Ah - good to know. I'll add that in.
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.
Of course, it all gets screwed up by Py3.12 saying that the preferred syntax is now type X = Y
...
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.
Also - we can't avoid doing the import in the TYPE_CHECKING block unless we make typing-extensions a base-level dependency (rather than a dev dependency). I'm hesitant to add a runtime requirement that by definition does nothing at runtime.
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.
Also - we can't avoid doing the import in the TYPE_CHECKING block unless we make typing-extensions a base-level dependency (rather than a dev dependency). I'm hesitant to add a runtime requirement that by definition does nothing at runtime.
hmm....it'll probably make more sense to discuss this in #2252....I have thoughts but they don't need to burden this PR :)
if sys.version_info < (3, 10): | ||
from typing_extensions import TypeAlias | ||
else: | ||
from typing import TypeAlias |
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.
You can leave as-is if you want (since I'll have to handle all this in #2252) but you don't have to worry about a conditional import. pyupgrade
will automatically switch to using only typing
to import TypeAlias
once 3.9 is no longer supported.
Adds an OptionContainer widget for iOS.
For the most part, it's exactly the same as what you'd expect on desktop, except that:
-iOS
suffix for test icons. For the purposes of this test, it doesn't matter because iOS is the only platform using icons for OptionContainer; but it highlights the need for platform-specific icon handling. In particular, iOS tab icons are only alpha masks, and if they're too big, they won't be resized. I have some ideas for how this can be handled by Toga which I'll push in a separate PR.PR Checklist: