Enable or Disable tab in optionContainer #832

ignaciocabeza · 2020-03-21T04:43:29Z

Signed-off-by: Ignacio Cabeza [email protected]

Added method to enable or disable an option of the OptionContainer for Cocoa
This is a WIP.

import toga
from toga.constants import COLUMN, ROW
from toga.style import Pack

class ExampleApp(toga.App):
        
    def on_enable(self, button):
        self.optioncontainer.set_option_enabled(1, True)

    def on_disable(self, button):
        self.optioncontainer.set_option_enabled(1, False)

    def startup(self):
        self.main_window = toga.MainWindow(title="Test Window")
        self.button = toga.Button('enable tab 1', on_press=self.on_enable)
        self.button2 = toga.Button('disable tab 1', on_press=self.on_disable)

        box1 = toga.Box()
        box2 = toga.Box()
        self.optioncontainer = toga.OptionContainer()
        self.optioncontainer.add('Option1', box1)
        self.optioncontainer.add('Option2', box2)
        
        controls = [
            self.button,
            self.button2,
            self.optioncontainer
        ]

        self.box = toga.Box(children=controls, style=Pack(direction=COLUMN, flex=1))
        self.main_window.content = self.box
        self.main_window.show()

def main():
    return ExampleApp('Test', 'org.beeware.widgets.testing')

if __name__ == '__main__':
    app = main()
    app.main_loop()

I think a better way would be to have a OptionItem class and store all instance of OptionItem inside of an OptionContainer and use it in this way:

...
self.optioncontainer.add('Option1', box1)
...

self.optioncontanier.get(1).enabled = False
self.optioncontanier.get(1).label = "I want to change the label

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

Signed-off-by: Ignacio Cabeza <[email protected]>

freakboy3742

Definitely a good suggestion for an API extension.

I agree with your suggested "better" API though - we need a wrapper object at the interface layer that can serve as a proxy for issuing commands on the implementation backend.

However, instead of using get(), I'd suggest overloading the __getitem__ protocol, and then defining property getters and setters for the features of interest:

table.option[3].enabled = True
table.option[0].title = 'Something new'

This would also allow (potentially) for lookup by name, as well as by index.

However, this API only needs to exist at the interface layer. The underlying implementation (cocoa, etc) can expose an interface similar to the one you've described here, where the API can require providing an index on set_option_enabled etc methods.

ignaciocabeza · 2020-04-01T10:27:24Z

I was reading the PRs and should I wait until #821 before start?

freakboy3742 · 2020-04-01T23:32:06Z

@ignaciocabeza In an ideal world, I agree it would be good to avoid working on this until #821 lands; however, FLOSS isn't an ideal world :-) Since everyone is a volunteer, it's difficult to establish when anyone will have the time to finish a patch, unless they're actively working on it (by which I mean we're seeing near daily updates).

I'll drop a comment on #821 mentioning that you're looking at this; but otherwise, I'd say you're free to proceed, and whoever lands second will have some merging work to do.

Signed-off-by: Ignacio Cabeza <[email protected]>

ignaciocabeza · 2020-04-04T04:12:25Z

Hi, I did some implementation of what we talked. I don't know if exactly as you described (my programming fundamentals are a little bit rusty).

Edit: and I added an example

ignaciocabeza · 2020-04-04T04:13:40Z

src/core/toga/widgets/optioncontainer.py

+    @enabled.setter
+    def enabled(self, value):
+        self._enabled = value
+        self._container.interface._impl.set_option_enabled(self._index, value)


I'm not sure if this is a correct use of toga or I'm breaking some toga design principle

The general idea of "invoke a method on the impl" is exactly the right idea.

The only criticism I'd have would be around how you get to the "_impl" in the first place.

freakboy3742

This is really good stuff - definitely heading in the right direction. I know there's a lot of comments inline, but most of them are relatively simple changes, mostly consequences of exactly how the OptionItem is being constructed. The general approach you've taken is dead on.

freakboy3742 · 2020-04-04T05:03:16Z

src/core/toga/widgets/optioncontainer.py

+    @enabled.setter
+    def enabled(self, value):
+        self._enabled = value
+        self._container.interface._impl.set_option_enabled(self._index, value)


The general idea of "invoke a method on the impl" is exactly the right idea.

The only criticism I'd have would be around how you get to the "_impl" in the first place.

freakboy3742 · 2020-04-04T05:04:28Z

src/core/toga/widgets/optioncontainer.py


+class OptionItem:
+    def __init__(self, container, label, widget, enabled):
+        self._container = container


The name "container" isn't quite right here. The actual object will be an OptionList, which will be owned by an OptionContainer.

That said, having a link to the list isn't especially helpful - you actually want a reference to the optioncontainer interface.

This variable should also be "None by default", and only assigned when the item is assigned to an optionList. Same goes for self._index.

freakboy3742 · 2020-04-04T05:05:43Z

src/core/toga/widgets/optioncontainer.py

+class OptionItem:
+    def __init__(self, container, label, widget, enabled):
+        self._container = container
+        self._index = len(container)


This should be a property of assignment, not construction. If you were to insert a tab at position N, "index" will only be correct if N is the last position in the list.

freakboy3742 · 2020-04-04T05:06:33Z

src/core/toga/widgets/optioncontainer.py

+        self._index = len(container)
+        self._label = label
+        self._widget = widget
+        self._enabled = enabled


Rather than having a duplicate of this data, it would be preferable to have it read directly from the actual widget. Is it possible to read the enabled/disable state from the widget itself?

This looks like it's still an issue.

The problem I'm facing here is that I'm not saving OptionItem Widget _impl anywhere. This OptionItem class is only saving the widget (self._widget) inside the OptionItem.

So, to access to label and enabled values I have to create some methods in the implementation layer like for example get_label and is_enabled and at the moment I only have OptionContainer implementation class to make this methods.

But rather than create that methods in the OptionContainer, It would be better have an OptionItem in the implementation layer, to put all the specific OptionItem functionality detached of the OptionContainer. Do you think this is the best way to do it?

I'm not sure I follow. The OptionItem has a link to the OptionContainer interface, from which you can get to the OptionContainer impl; If the impl layer has an set_option_enabled(index, state) and is_option_enabled(index), that would allow the interface to directly access the state of the widget by providing the context that is needed (i.e., the index/label) to answer the request.

The key insight here is that the API at the impl layer is purely functional - it's whatever lets us get the job done. It doesn't need to match the public interface-layer API. We can have methods on the impl layer that let us achieve functional goals, but don't have any directly corresponding method on the interface layer. That's one of the benefits of a layered API - we can expose a completely clean API to the end user, but internally use whatever API makes sense to bridge the differences between platforms.

We're already doing a similar thing with Tree/Table widgets. The public API for the Tree/Table is based around data sources; however, the impl layer talks about adding rows, refreshing rows, etc.

Does that make sense?

The key insight here is that the API at the impl layer is purely functional
I based on this to make changes, I wasn't sure about what do or not to do in impl layer but now I think it's clear. Thanks!

src/core/toga/widgets/optioncontainer.py

freakboy3742 · 2020-04-04T05:13:02Z

src/core/toga/widgets/optioncontainer.py

+                opt.enabled for opt in self._content
+                if opt != self._content[index]
+            ]
+        )


I'm not sure I follow what this logic is for. You can't remove a tab if any of the tabs are disabled? That doesn't seem to make any sense to me? At the very least, an explanatory comment is needed.

If we don't have this check and we delete option3 in this scenario:

option1(disabled)

option2(disabled)

option3(enabled)

Remove will work OK but option2 will be focused and disabled at the same time which I think is worse than don't allow remove it. This behaviour at least is happening in macos.

Ah - that makes sense, but it's not exactly intuitive.

I'm not completely convinced "disabling the tab you're on" is as bad as you suggest; but if we are going to support this edge case, I'd argue it would make more sense to force focus onto the first non-disabled tab after the one you've selected (wrapping around to the first tab if that isn't an option). That would also mean that if there isn't at least one enabled tab to land on, you can't disable a tab, and attempting to do so should raise an error.

As it happens, I was able to get into this state via a different path: in your demo app, if you delete all the tabs but one, and press the "toggle first button", you end up with a "disabled but enabled" tab.

In that state, you can also press the delete tab button, and it does nothing. That's effectively the "nowhere to land" case I described previously.

src/core/toga/widgets/optioncontainer.py

ignaciocabeza · 2020-04-15T10:33:32Z

Hi @freakboy3742. I did some changes according to your recommendations

freakboy3742

A couple more tweaks, mostly around the remove API. It's looking good, though!

examples/optioncontainer/optioncontainer/app.py

freakboy3742 · 2020-04-18T05:52:42Z

src/core/toga/widgets/optioncontainer.py

+        self._index = len(container)
+        self._label = label
+        self._widget = widget
+        self._enabled = enabled


This looks like it's still an issue.

src/core/toga/widgets/optioncontainer.py

freakboy3742 · 2020-04-18T06:02:08Z

src/core/toga/widgets/optioncontainer.py

+                opt.enabled for opt in self._content
+                if opt != self._content[index]
+            ]
+        )


Ah - that makes sense, but it's not exactly intuitive.

I'm not completely convinced "disabling the tab you're on" is as bad as you suggest; but if we are going to support this edge case, I'd argue it would make more sense to force focus onto the first non-disabled tab after the one you've selected (wrapping around to the first tab if that isn't an option). That would also mean that if there isn't at least one enabled tab to land on, you can't disable a tab, and attempting to do so should raise an error.

As it happens, I was able to get into this state via a different path: in your demo app, if you delete all the tabs but one, and press the "toggle first button", you end up with a "disabled but enabled" tab.

In that state, you can also press the delete tab button, and it does nothing. That's effectively the "nowhere to land" case I described previously.

src/core/toga/widgets/optioncontainer.py

ignaciocabeza

I made some new changes. I re made optioncontainer example and I changed some issues you mentioned in the previous review.

src/core/toga/widgets/optioncontainer.py

src/cocoa/toga_cocoa/widgets/optioncontainer.py

freakboy3742

We're definitely getting closer; but we've still got some edge cases around enable/disable/remove logic.

In the last iteration I reviewed, you had some logic around validating whether a tab removal was legal; that logic seems to have been removed. I think that logic was necessary; it just had some edge cases that weren't quite right.

src/cocoa/toga_cocoa/widgets/optioncontainer.py

src/core/toga/widgets/optioncontainer.py

src/dummy/toga_dummy/widgets/optioncontainer.py

src/cocoa/toga_cocoa/widgets/optioncontainer.py

…ency

ignaciocabeza · 2020-04-28T10:34:43Z

Hi!,

I covered 3 edge cases in cocoa implementation:

Don't allow remove if there is only one tab left
Don't allow disable if there is only one tab left
Don't allow disable tab if tab is selected
(Please review english messages because english is not my first lang)

And I renamed methods for consistency.

freakboy3742

We're getting there...!

src/cocoa/toga_cocoa/widgets/optioncontainer.py

freakboy3742 · 2020-04-29T06:58:26Z

src/cocoa/toga_cocoa/widgets/optioncontainer.py

-            raise Exception('Disable selected Option is not allowed')
+            raise OptionException('Selected Option cannot be disabled')
+
+        if self.native.numberOfTabViewItem == 1:


This took me a moment to understand; it relies on the implied knowledge that if there's only one item, it must be enabled. While this is absolutely correct, it's worth being explicit about this by adding an "if not enabled" - either in the code, or in a code comment explaining the edge case.

There's also once more edge case, which is the more general case of "only 1 tab" - the case where you're disabling the last enabled tab - (i.e., tab A B and C all exist and are disabled, tab D exists and is selected and enabled; you can't disable D).

I think this case is covered by raise OptionException('Currently selected option cannot be disabled'). if you disable A,B,C then D has to be selected. I tried in example app and exception is raised.

src/gtk/toga_gtk/widgets/optioncontainer.py

…po and fixed disable condition

freakboy3742

We're getting closer, but I'm still finding edge cases in my testing, mostly around deleting tabs that are selected, and deleting the last tab.

These instructions might be a little confusing because of the discrepancy between the tab name "option 1" and it's position "0"; I think it would be worth modifying the example so that the name of the tab and the position in the pulldown is the same (it doesn't really matter which way you fall, as long as the naming is consistent).

Case 1

Start app
Mark index 1 disabled
Mark index 2 disabled
Remove option 0

App doesn't crash; Option 2 and 3 remain, in a disabled state; box1 content remains

Case 2

Select option3
Disable index 1
Delete index 2

Option 1 is marked as selected; but the widget content is still box 3.

src/cocoa/toga_cocoa/widgets/optioncontainer.py

examples/optioncontainer/optioncontainer/app.py

ignaciocabeza · 2020-05-09T01:35:46Z

We're getting closer, but I'm still finding edge cases in my testing, mostly around deleting tabs that are selected, and deleting the last tab.

These instructions might be a little confusing because of the discrepancy between the tab name "option 1" and it's position "0"; I think it would be worth modifying the example so that the name of the tab and the position in the pulldown is the same (it doesn't really matter which way you fall, as long as the naming is consistent).

Case 1

Start app

Mark index 1 disabled

Mark index 2 disabled

Remove option 0

App doesn't crash; Option 2 and 3 remain, in a disabled state; box1 content remains

Case 2

Select option3

Disable index 1

Delete index 2

Option 1 is marked as selected; but the widget content is still box 3.

I resolved case Case 2 no letting delete index 2 if it's selected.

freakboy3742

Thanks for those updates. Thinking about it some more, I think we've been over-thinking things. If we make any attempt to disable or remove the currently selected tab, all the edge cases we've previously described disappear. The only cost is the behavior where deleting the current tab would cause a different tab to be selected; but it's not immediately clear to me that this is universally desirable behavior anyway.

I also noticed that in order to be useful, the exception needs to be in the interface layer.

I've made those changes, along with a couple of other small cosmetic fixes.

Thanks for the contribution - another great addition to the Toga API!

ignaciocabeza · 2020-05-18T11:07:12Z

Thanks for merging!. I tried the example after your changes and you're right about over-thinking. Thanks a lot for the 2 month feedback.

enable/disable tab in optionContainer (wip)

3059ba3

Signed-off-by: Ignacio Cabeza <[email protected]>

freakboy3742 requested changes Mar 22, 2020

View reviewed changes

freakboy3742 mentioned this pull request Apr 1, 2020

Add iOS OptionContainer implentation #821

Closed

4 tasks

ignaciocabeza added 2 commits April 4, 2020 12:06

first working implementation

bba51b9

added the actual code of the implementation and not just the exxample

b7a2620

Signed-off-by: Ignacio Cabeza <[email protected]>

ignaciocabeza commented Apr 4, 2020

View reviewed changes

freakboy3742 requested changes Apr 4, 2020

View reviewed changes

changes after code review

f7c38d1

freakboy3742 requested changes Apr 18, 2020

View reviewed changes

fixes, changes in example app and small fix in selection widget

7c39169

ignaciocabeza commented Apr 24, 2020

View reviewed changes

src/core/toga/widgets/optioncontainer.py Show resolved Hide resolved

src/cocoa/toga_cocoa/widgets/optioncontainer.py Outdated Show resolved Hide resolved

freakboy3742 requested changes Apr 25, 2020

View reviewed changes

freakboy3742 mentioned this pull request Apr 25, 2020

Get the id of the selected Tab View on the OptionContainer [Core] #197

Closed

improve edge cases handling exceptions and rename methods for consist…

3e814b0

…ency

fix gtk tests

d3e03cf

freakboy3742 requested changes Apr 29, 2020

View reviewed changes

fixed gtk not_implemented, changed OptionException messages, fixed ty…

9263958

…po and fixed disable condition

freakboy3742 requested changes Apr 30, 2020

View reviewed changes

src/cocoa/toga_cocoa/widgets/optioncontainer.py Outdated Show resolved Hide resolved

examples/optioncontainer/optioncontainer/app.py Outdated Show resolved Hide resolved

control more edge cases, fix example option names

80c9c0b

freakboy3742 approved these changes May 16, 2020

View reviewed changes

freakboy3742 added 3 commits May 16, 2020 15:03

Small tweaks to the implementation and example.

0ada8f0

Updated Winforms API with mocks of non-implemented APIs.

c91c01c

Cleanups to GTK and Cocoa implementations.

ce43649

freakboy3742 merged commit 803bcf4 into beeware:master May 16, 2020

mhsmith mentioned this pull request Aug 9, 2023

[widget Audit] toga.OptionContainer #1996

Merged

11 tasks

Uh oh!

Enable or Disable tab in optionContainer #832

Enable or Disable tab in optionContainer #832

Uh oh!

Conversation

ignaciocabeza commented Mar 21, 2020

PR Checklist:

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

ignaciocabeza commented Apr 1, 2020

Uh oh!

freakboy3742 commented Apr 1, 2020

Uh oh!

ignaciocabeza commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ignaciocabeza Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ignaciocabeza commented Apr 15, 2020

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ignaciocabeza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ignaciocabeza commented Apr 4, 2020 •

edited

Loading

ignaciocabeza Apr 15, 2020 •

edited

Loading