ModalView: Updating ModalView to improve theming

[snuq]

Some simple proposed changes to the ModalView class to make its background_color behave more closely to the same variable in other classes in kivy. Also, added overlay_color variable to allow independent setting of the 'fade out' color when a popup is activated.

Note that the doc string for background_color is copied from the Button class, hopefully they parse correctly, I do not know anything about the documentation format or how to test it.

[welcome]

Thanks for opening your first pull request here! 💖 Please check out our contributing guidelines.

[snuq]

i believe this will fix
#2198
as well, tho it is difficult to tell without the user's code available.

[inclement]

Is there a particular reason to use canvas.before? I think canvas is probably a better choice.

[snuq]

I was having trouble with it since the usual way to change the background image via canvas is using canvas.before, and when you try to use a canvas.before on a Popup (which is a child class of ModalView), it is completely overlayed by this, and the only way to effectively nullify this is to use a transparent png as the background image.

[inclement]

As @matham mentioned on discord, the canvas is itself ordered background to foreground, the point of canvas.before and canvas.after is to bypass that ordering when adding instructions in different places (in which case managing the single flattened canvas list is inconvenient). When you make a widget like this, you have full control over the ordering, so it's usually best to do everything on the normal canvas.

This is especially important for Kivy's own widgets, because these are building blocks that many apps will use. They generally want to avoid using the canvas.before and canvas.after specifically because that makes it harder for users to do their own drawing before and after the Kivy widget. Also note that 'drawing' encompasses more than displaying shapes, but also context instructions, see below.

Your change here is well motivated, but I think it's still the worse choice - if anything, the fact that the ModalView draws an opaque background image by default may be the bad part of the api. As I would see it, this change breaks the normal convention: you've specifically made canvas.before draw after the widget, which is the opposite of its normal behaviour. I understand why it may be initially surprising that drawing a background before everything else means it's behind the image that the widget displays...but isn't that what you should expect? That's why it's called canvas.before!

There may be other practical issues. For instance, users would normally expect that using a Rotate or Translate instruction on canvas.before would affect the widget, but this would not work here.

For these reasons, I think using canvas is the most consistent and preferable option.

[inclement]

Also, setting the background_color to have 0 opacity should be a more convenient alternative to using a transparent png.

[snuq]

ok, you are right about the background_color, i was thinking about adding this with the way it used to behave (i had to do the hack of making the background transparent on my app to get it to work properly).
What do you think about making the opacity fadeout thing a canvas.before, and the background image itself canvas? Not asking for any usability reasons, just does that seem to make more sense?

[inclement]

My initial reaction is that even the fadeout should probably be on the canvas rather than canvas.before, simply because I don't see a major reason not to do it, but if you have a good reason for it then that could be fine.

[snuq]

Honestly, I dont have a good reason for putting it on canvas.before, just thought it made sense so I would suggest it, either way is fine by me.

[snuq]

Changed it back to just use canvas.

[inclement]

Nice work @snuq, thanks for your patience in working through the issues.

I'm mildly concerned about this being a breaking change, but have no major objection to merging it if we're happy with that. @tshirtman how does the break look to you?

[misl6]

I had the oppurtunity to check this PR on a project I'm working on, on my side it's working fine.

[tshirtman]

Nice work @snuq, thanks for your patience in working through the issues.

I'm mildly concerned about this being a breaking change, but have no major objection to merging it if we're happy with that. @tshirtman how does the break look to you?

i think this change is indeed for the better, the current way certainly prevent some reusability of the widget, and the api is not clear, since this is a 2.0 i think i'm ok with such breaks. But i don't have a lot of kivy apps to maintain currently, so i'm not very impacted.

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉