WindowSDL: Add opacity feature

[mak8kammerer]

Hello everyone!

Today I want to add opacity feature to window.
Based on this on this PR: #5169.

Several notes:

• Kivy won't perform range checking on opacity property's value, and it won't raise an error if you assign value outside the valid range of [0.0 - 1.0]. Instead, the Window provider will clamp the value to the valid range, ensuring that the opacity remains within [0.0 - 1.0].

• Opacity property is cached, which means that the value dispatching event will only be raised when you set a new value that is different from the current cached value. This behavior ensures that unnecessary events are not dispatched when the value remains the same.

Maintainer merge checklist

• Title is descriptive/clear for inclusion in release notes.
• Applied a Component: xxx label.
• Applied the api-deprecation or api-break label.
• Applied the release-highlight label to be highlighted in release notes.
• Added to the milestone version it was merged into.
• Unittests are included in PR.
• Properly documented, including versionadded, versionchanged as needed.

[misl6]

Nice! How about adding some tests to our suite that check everything works as expected?

"Everything works as expected":

• opacity property changes
• opacity < 1.0 or opacity > 0. check is performed and warning log is dispatched.

I do not expect you to test that the window is respecting the opacity set via SDL_SetWindowOpacity, as I expect this test has already been performed on SDL side.

FYI: We're trying to encourage contributors to add tests in 2.3.0, as an effort to make Kivy more stable and well-tested (so we can avoid potential issues in future)

[mak8kammerer]

OK, I'll add the tests.

[misl6]

Hi @mak8kammerer !

The PEP8 checks are failing, can you take care of it?

[misl6]

Ouch! Are failing again 😅

[mak8kammerer]

I see, I'll figure it out now.

[mak8kammerer]

@misl6, can you run checks again?

[pythonic64]

SDL2 will clamp the value if less than 0 or greater than 1, so I would remove range testing from this method (and logger warning) and let specific window implementation decide on what to do. Since SDL2 will clamp there is no need to range test in WindowSDL, but just test for errors https://github.com/kivy/kivy/pull/8296/files#r1248874160

[pythonic64]

@misl6 Do you agree?

[misl6]

@pythonic64 thanks for chiming in! ❤️

I partially agree:

• Testing for errors (and logging it) via SDL_GetError is absolutely something we should have. (As for any reason, even if the value is in the supported range, something bad may happen, and the user/developer needs to be informed)

• Defining a supported range on WindowBase looks like a good approach instead, as will force us in the future to standardize the opacity range along all the window providers. (the provider-specific implementation will take care of translating the 0.0 - 1.0 range to a supported value). By not defining a supported range, the risk is that in the future we may introduce a window provider with 0-100 range, which is incompatible with the 0.0-1.0 range.
  By defining a supported range, Kivy is responsible to maintain compatibility by applying the needed translations.

[pythonic64]

@misl6 I agree on standardizing opacity range to 0.0 - 1.0 but imho that should be (and in this pull request already is) stated only in the docs. Similarly, we don't do range test for each component in ColorProperty, but we do state supported range in the docs. If in the future we have another window provider we will still respect the defined range from the docs.

Therefore, code in _set_opacity should just call self._set_window_opacity(opacity), without doing a range test, as developers in most cases will use values from the defined range. Specific WindowSDL implementation will pass the value and do the error check. Similarly, if a new window provider uses [0-100] range then we would upscale to that range, pass the value and do the error check.

[misl6]

I agree on standardizing opacity range to 0.0 - 1.0 but imho that should be (and in this pull request already is) stated only in the docs. Similarly, we don't do range test for each component in ColorProperty, but we do state supported range in the docs. If in the future we have another window provider we will still respect the defined range from the docs.

Yep. Makes sense!

@pythonic64 and @mak8kammerer feel free to ping me when you need a final review or to run tests (as @mak8kammerer is a new contributor, and workflow runs need to be manually approved) 😃

[mak8kammerer]

@pythonic64, I agree with your suggestions.
However, calling the SDL_SetWindowOpacity function with the opacity parameter outside the range [0.0, 1.0] returns 0 (everything is fine). For example, setting opacity=1.5 will set the window opacity to 1.0 and SDL_SetWindowOpacity returns 0.
I think range testing should be left.
I agree with other proposals.

[pythonic64]

@mak8kammerer SDL will clamp the value, that's stated in the docs, "Remarks" section: https://wiki.libsdl.org/SDL2/SDL_SetWindowOpacity#remarks .

In our discussion above we agree that range should be specified in the docs only as we don't expect users assigning values outside of that range. Since SDL is clamping the value that's fine, but if in the future we have another window provider which doesn't clamp the value but raises an error then we will decide in that window specific implementation to either:

1. clamp and set the clamped value
2. catch and log the error, leaving the current opacity value unchanged

Widget.opacity is an another example where we don't do range check and assigning values outside of [0.0-1.0] does not raise an error.

[pythonic64]

Regarding tests:
I suggest using setUp/tearDown to store/restore previous opacity value and LoggerHistory.history list and in that case we need a new class, so @mak8kammerer we should restore WindowOpacityTest and test for these cases:

1. test if opacity is supported -> no error log on opacity change
2. (if supported) test values within [0, 1] range
3. (if supported) test if values are clamped

[pythonic64]

@mak8kammerer Can you add more info in the description on how the opacity feature is implemented (eg. opacity is a property, cached, dispatch value on change)? Also, I would suggest changing the title to "WindowSDL: Add opacity feature".

[mak8kammerer]

@pythonic64, work is done.

[pythonic64]

@mak8kammerer I meant in the description of this pull request #8296 (comment) . Current description of the property is fine and we don't have to add anything to it.

Also, did you missed this comment #8296 (comment)?

[pythonic64]

Request looks good to me.

@misl6 Can you approve and merge?

[misl6]

@pythonic64 and @mak8kammerer , here return self._win.set_window_opacity(opacity) should not be self._set_window_opacity(opacity) ?

[mak8kammerer]

@misl6, look this

[misl6]

Seems that @pythonic64 was suggesting to change it to return self._set_window_opacity(opacity) (which is correct), and not return self._win.set_window_opacity(opacity) (at the link you provided)

[mak8kammerer]

Sorry, my blame.

[mak8kammerer]

@misl6, fixed! Thank You!

[pythonic64]

@misl6 Correct, that line should be return self._set_window_opacity(opacity)

[mak8kammerer]

@misl6, there was clearly something wrong with me. Yesterday, while editing, I lost return.

[pythonic64]

@misl6 Correct, that line should be return self._set_window_opacity(opacity)

[misl6]

LGTM. Thank you!

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉

[pythonic64]

@mak8kammerer Thank you for getting this feature done.

@misl6 I think you can close #5169.