[Qt] Add OptionContainer widget

[corranwebster]

This adds the OptionContainer widget to the Qt backend.

Implementation seems fairly straightforward, as the Qt API matches the index-based approach to adding tabs.

To get Icon support working for the tabs we need to be able to do a reverse-lookup on the QIcons–we can't do this on the instances, because we get a new QIcon instance on the Python side when we ask for the icon from the tab–but Qt provides a QIcon.cacheKey() method which provides an identifier based on the actual contents. So we change the IMPL_DICT lookup table to use this as the dictionary key.

There is additional question related to #3923 which is whether icon files should search for a backend-related name rather than a platform-related name (ie. "new-tab-qt.png" instead of "new-tab-linux.png"). I'm not sure what's the right thing here.

This is split out from #3966 for ease of review.

Ref #3914.

To Do:

• Screenshot
• Comparison of internal widget layout with other backends. Internal container widgets aren't resizing to match size of OptionContainer.
• Maybe see if we can get Icon support.

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

[johnzhou721]

This mostly makes sense to me -- good job on enforcing minimum size in content_refreshed and using that to form the size hint.

Some notes, though.

[johnzhou721]

Really nice fix!!! I totally did not know this.

Since IMPL_DICT is now usable, perhaps we could try replacing the custom interface caching in qt/widgets/button.py with this mechanism.

[corranwebster]

In retrospect I think there's the potential for a little weirdness if you have two Icons with different source files which have the same content as pixels (and so same cacheKey). It will give you back the most recently loaded Icon object, not necessarily the one which was used in the widget. I don't think that's a problem, since the actual icon content is identical, but I don't know for sure.

I'll have a look at the button caching since that might give an idea about the constraints.

[corranwebster]

So I dug a bit deeper into the Qt code, and the good news is that if the QIcon cacheKey is basically a serial number (incremented for each new QIcon instance) together with a version number (the number of times the content has been changed), so there should never be collisions and two different icons should never have the same cache key, even if the content is the same.

And yes, button caching could use this rather than storing the icon itself.

[freakboy3742]

Regarding the Icon question - We may need to add a layered approach here; i.e., looking for -qt, then -linux, then an icon with a suffix. However, we don't need to solve that problem here; there are some other issues with icon lookup (e.g., #3564) that need some attention; so the whole icon-lookup strategy likely needs some work.

[corranwebster]

I think this is ready for re-review.

[johnzhou721]

@corranwebster Howdy! (just found out you're a former Texan... it's so hot here!)

Ack; I've got some other work to do, but I'll take a look at this tomorrow.

Also note that the core team (which I'm not a part of) is on holiday break, so expect official response times to be up to 8 days (as they will be checking weekly).

[johnzhou721]

Mostly looks good; all functionality is fine.

Some final layout notes inline -- not sure if this'd be a requirement to merge because the stuff doesn't work reliably on all other backends either.

(This fix is also needed on Cocoa but I'll suggest it here since I'd rather new code we add be correct.)

[johnzhou721]

This should be minimumSizeHint to take properly into account the minimum sizes of the child widgets, not just sizeHint.

[johnzhou721]

1 -- this should be maximum, not minimum to ensure that all tabs fit; 2 -- is this neccessary? Qt minimumSizeHint hinting already takes the maximum of all minimum sizes, so we could just set the minimum size of each subcontainer to the determined minimum size of that subcontainer only.

[johnzhou721]

After you set the minimum size of the native widget, I'd suggest

    prev_intr_width, prev_intr_height = self.interface.intrinsic.width, self.interface.intrinsic.height
    self.rehint()
    intr_width, intr_height = self.interface.intrinsic.width, self.interface.intrinsic.height
    if (prev_intr_width, prev_intr_height) != (intr_width, intr_height):
        asyncio.get_running_loop().call_soon_threadsafe(self.interface.refresh)

-- this queues up a second refresh if the intrinsic size of the widget ever changes, so we could have appropriate sizing for the OptionContainer. The interface refresh will perform a layout on the entire tree outside the optioncontainer with correct minimum size for the OptionContianer widget, which will enforce the minimum size properly.

One refresh is queued in the interface code for adding/removing tabs; however, a second refresh is needed because the first refresh will use an outdated minimum size hint, but it's neccesary for that refresh to happen so we could refresh the widget's contents as well.

Along with the minimumSizeHint I mentioned and if I change content_refreshed to this with proper asyncio import and linting, size hinting shuold all work:

    def content_refreshed(self, container):
        container.native.setMinimumSize(container.content.interface.layout.min_width, container.content.interface.layout.min_height)
        prev_intr_width, prev_intr_height = self.interface.intrinsic.width, self.interface.intrinsic.height
        self.rehint()
        intr_width, intr_height = self.interface.intrinsic.width, self.interface.intrinsic.height
        if (prev_intr_width, prev_intr_height) != (intr_width, intr_height):
            asyncio.get_running_loop().call_soon_threadsafe(self.interface.refresh)

[johnzhou721]

@corranwebster The code mostly looks good; I do have reservations about the no-cover though — however I think as part of this PR we can leave it as is and I’ll try to submit a PR with the equivalent fix for other platforms
and add a test later.

[johnzhou721]

FYI -- the no-cover can be removed ocne #4010 lands.

[kattni]

Hi @corranwebster and @johnzhou721. It looks like this is ready for the team to review. Can you please verify? Thanks!

[johnzhou721]

@kattni This is stacked on #4010 in order to merge since the no cover has to be removed, however, yes, this needs a review.

Thank you!

[corranwebster]

@kattni @johnzhou721 I've merged with main and removed the no-cover, so assuming tests pass this is ready for review by the core team, I think.

[corranwebster]

@kattni @johnzhou721 I've merged with main and removed the no-cover, so assuming tests pass this is ready for review by the core team, I think.

Ooops, it looks like #4010 hasn't been merged. So coverage will fail.

Although review by the core team would still be useful even with the known issue of coverage.