Skip to content
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

Slider dragging signals #3884

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Slider dragging signals #3884

wants to merge 3 commits into from

Conversation

Elv13
Copy link
Member

@Elv13 Elv13 commented Dec 31, 2023

Copy of #3533

Changed the :is_dragging() method to be a property and fix luacheck warnings.

@sclu1034 Are you fine with this? This address a comment I left (and then forgot about) in the other PR.

When trying to hook up the slider to an external value, one would
usually have two data streams:

- `property::value` -> set external value
- external value changed -> `.value = new_value`

The problem is that without manual intervention, these two streams form
a loop, as setting `.value` also emits `property::value`.

The new set of signals is disconnected from the `value` property and its
signal and allows for more fine grained inspection of the dragging
state.

Signed-off-by: Lucas Schwiderski <lucas@lschwiderski.de>
Signed-off-by: Lucas Schwiderski <lucas@lschwiderski.de>
@Elv13 Elv13 changed the title Copy of https://github.com/awesomeWM/awesome/pull/3533 Slider dragging signals Dec 31, 2023
Copy link

codecov bot commented Dec 31, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1f7ac8f) 91.22% compared to head (eaff8aa) 91.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3884      +/-   ##
==========================================
+ Coverage   91.22%   91.25%   +0.03%     
==========================================
  Files         927      928       +1     
  Lines       59450    59522      +72     
==========================================
+ Hits        54232    54317      +85     
+ Misses       5218     5205      -13     
Flag Coverage Δ
gcov 91.25% <98.61%> (+0.03%) ⬆️
luacov 93.91% <98.61%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
tests/test-wibox-widget-slider.lua 100.00% <100.00%> (ø)
lib/wibox/widget/slider.lua 96.85% <85.71%> (+8.81%) ⬆️

... and 2 files with indirect coverage changes

In CI, the wibox doesn't always seem to be in the same location.
So before the actual test steps, we need to find it first.
@sclu1034
Copy link
Contributor

sclu1034 commented Jan 1, 2024

I mean, you already went as far as creating an entire copy of the PR to implement your own version before I even got to answer that question, which appears to me like my answer won't really affect much.
But for posterity, no I'm not. Making this a "property" doesn't make sense to me.

Users expect to be able to set a value for a property (except for the client properties where X11 dictates otherwise) and to get a corresponding signal on change (which still applies for those X11 properties).
But neither applies here. Making this a property just turns the direct function call into a function call hidden by metatables and the user not typing ().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants