Slider Handle Widget #3699
Slider Handle Widget #3699
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3699 +/- ##
==========================================
- Coverage 90.92% 90.89% -0.04%
==========================================
Files 896 896
Lines 56630 56693 +63
==========================================
+ Hits 51493 51533 +40
- Misses 5137 5160 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks for this. It looks like an useful feature. I left a few minor comments.
For the tests, if you want, you can create some unit tests in spec/ and run them using make check-unit. You need busted to be installed. We don't have any for the slider right now. See spec/wibox/widget/slider_spec.lua for a minimal example.
But an alternative is to create a couple documentation examples based tests. They are not as fancy, but a lot easier to do and they provide much needed code examples for the users. You can copy paste an existing one and modify it. Pay attention to --DOC_HIDE (_START/_END) and __DOC_NEWLINE. They are preprocessed to help format the examples to remove the boilerplate. We are trying to have at least one of those for every properties.
The automatic bar for the tests right now is 91% overage. This should be rather easy here. If the margin type if gets mutualized in the setter, then a simple test should hit nearly 100% of the code.
Bonus points if you can implement bar_widget for the slider (and maybe progressbar too?). The code would probably be 95% identical and would help consistency across those modules.
|
Oh, and also please install Checking lib/wibox/widget/slider.lua 4 warnings
lib/wibox/widget/slider.lua:517:15: value assigned to variable 'x' is overwritten on line 543 before use
lib/wibox/widget/slider.lua:517:18: value assigned to variable 'y' is overwritten on line 544 before use
lib/wibox/widget/slider.lua:531:17: value assigned to variable 'handle_height' is unused
lib/wibox/widget/slider.lua:536:17: value assigned to variable 'handle_height' is unused
Checking lib/wibox/widget/systray.lua OK |
|
I'll clean up my code and add the docs. I was considering doing a rewrite of the slider widget at some point rather than trying to edit what exists. I think the current widget is more focused on cario drawings, and a ground up redesign might be better. I'm open to anything though. |
It would be pretty hard to merge (API compatibility and duplication requirements). IMHO extending the current one benefits more users. What kind of feature would such a rewrite improve upon/add? Not saying the current widget is incredible. It has a strange history.
I think it's useful to provide both shape/cairo and sub-widgets in the same module. Most users are satisfied with shapes+colors, but might eventually want to upgrade to a declarative custom sub-widgets. Doing so without having to replace the code or adding a 3rd party widget library makes it a lot more straightforward. Having less overlapping modules also reduce the mental load when learning/experimenting. It doesn't make the internal implementation pretty, but I am usually fine with internal complexity if it makes the user experience better. |
|
Understandable. For the bar widget, how exactly would you like that to be implemented functionality-wise? Would you like it to span the container, or only fill the active portion? |
I just find the code to be a bit messy, and a refactor would make it easier to understand/maintain. I figure a refactor is vastly different from a complete rewrite, I just think the chance to clean it up a bit at some point might be valuable. |
Refactors are fine. Just make sure the public API and behavior don't have unexpected change. Depending on what kind of scope you want in the PR, maybe you want the initial version merged first? That still requires the doc and tests/example/wibox/widget/slider file.
Behave the same as the |
|
I'll work on the bar widget and the docs/tests. The refactor would definitely be a different pr. |
|
I'm currently working on the margin setters. How would I call them on the widget init? I set them up to format the |
Move This is already used in various other widgets where setters have some business logic. |
|
So how does the setter get called? |
AwesomeWM objects (both |
|
Ok, I added the |
lib/wibox/widget/slider.lua
Outdated
| (margins.left or 0) - (margins.right or 0) | ||
| handle_height = handle_height - | ||
| (margins.top or 0) - (margins.bottom or 0) |
There was a problem hiding this comment.
If we are going to use X times the margins.direction or 0 statement, we should definitely have local variables to handle these operations once for all.
There was a problem hiding this comment.
I'm not sure if I fully understand. What scope should the vars be in?
There was a problem hiding this comment.
Ok, just pushed some changes to the margin stuff. If you could take a look and see if it's what you were looking for.
|
Since the PR is still a work in progress, I'm converting it to a draft. |
|
|
Will these also add object properties? |
They don't add anything, they are just code examples used to generate images and (optionally) code for the doc. This allows us to unit test our examples and have global templates to maintain the images. If you clean your build directory, copy paste some existing files from the same directory and rebuild awesome, it should just work. Then you can modify the code to better suit the property you are demonstrating. |
|
I'm having and unreasonably difficult time getting -- The slider handle widget.
--
--@DOC_wibox_widget_slider_handle_widget_EXAMPLE@
--
-- @property handle_widget
-- @tparam widget|nil handle_widget
-- @propemits true false
-- @propbeautiful
-- @see wibox.widgetThe file I'm opening to check is |
|
I'm not 100% sure on the method documentation for |
|
Hold up, I forgot testing. |
|
The image is diff --git a/tests/examples/wibox/widget/slider/bar_widget.lua b/tests/examples/wibox/widget/slider/bar_widget.lua
index 96415c17f..d950b4e8b 100644
--- a/tests/examples/wibox/widget/slider/bar_widget.lua
+++ b/tests/examples/wibox/widget/slider/bar_widget.lua
@@ -20,4 +20,19 @@ local function gen(val)
}
end
+local l = wibox.layout {
+ gen(0), gen(3), gen(6), gen({
+ top = 12,
+ bottom = 12,
+ left = 12,
+ right = 12,
+ }),
+ forced_height = 30,
+ forced_width = 400,
+ spacing = 5,
+ layout = wibox.layout.flex.horizontal
+}
+
+parent:add(l)
+
--DOC_HIDE vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80
diff --git a/tests/examples/wibox/widget/slider/handle_widget.lua b/tests/examples/wibox/widget/slider/handle_widget.lua
index 29928c1b7..05d0c7567 100644
--- a/tests/examples/wibox/widget/slider/handle_widget.lua
+++ b/tests/examples/wibox/widget/slider/handle_widget.lua
@@ -20,4 +20,19 @@ local function gen(val)
}
end
+local l = wibox.layout {
+ gen(0), gen(3), gen(6), gen({
+ top = 12,
+ bottom = 12,
+ left = 12,
+ right = 12,
+ }),
+ forced_height = 30,
+ forced_width = 400,
+ spacing = 5,
+ layout = wibox.layout.flex.horizontal
+}
+
+parent:add(l)
+
--DOC_HIDE vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80However, "does things" here means this:
This is what tests are for ;) |
| @@ -307,7 +323,47 @@ for prop in pairs(properties) do | |||
| end | |||
| end | |||
|
|
|||
| --- Set the handle widget | |||
There was a problem hiding this comment.
Those @method sections are not required. The @property is enough. Otherwise, it will document how to set the widget twice. We only document some legacy setters which had more than 1 parameters. Everything else is documented using the property mechanism.
I think the margins aren't being created by their setter functions. in the setter function I set the fallback of 0. |
Yeah, it will need an |
|
that'll set it to 0, but it needs to build the table aswell. |
|
Where should I put that?
The args is always nil |
I fixed that in the diff below by adding diff --git a/lib/wibox/widget/slider.lua b/lib/wibox/widget/slider.lua
index 344f91e20..7becf2e9e 100644
--- a/lib/wibox/widget/slider.lua
+++ b/lib/wibox/widget/slider.lua
@@ -40,7 +40,7 @@ local slider = {mt={}}
--@DOC_wibox_widget_slider_handle_widget_EXAMPLE@
--
-- @property handle_widget
--- @tparam[opt=nil] widget|nil handle_widget The handle widget
+-- @tparam[opt=nil] widget|nil handle_widget The handle widget
--- The slider handle color.
--
@@ -323,14 +323,6 @@ for prop in pairs(properties) do
end
end
---- Set the handle widget
---
--- @method set_handle_widget
--- @tparam[opt=nil] widget|nil bar_widget Set the handle widget
--- @noreturn
-
-
-
function slider:set_handle_widget(value)
local changed = self._private.handle_widget ~= value
self._private.handle_widget = value
@@ -340,14 +332,6 @@ function slider:set_handle_widget(value)
end
end
---- Set the bar widget
---
--- @method set_bar_widget
--- @tparam[opt=nil] widget|nil bar_widget Set the widget that spans the active bar segment
--- @noreturn
-
-
-
function slider:set_bar_widget(value)
local changed = self._private.bar_widget ~= value
self._private.bar_widget = value
@@ -357,13 +341,6 @@ function slider:set_bar_widget(value)
end
end
--- Add some validation to set_value
-
---- Set the slider's value
--- @tparam number number Set value to number
--- @method set_value
--- @noreturn
-
function slider:set_value(value)
value = math.min(value, self:get_maximum())
value = math.max(value, self:get_minimum())
@@ -388,22 +365,9 @@ local function get_extremums(self)
return min, max, interval
end
--- TODO add single number param to margin setters
-
---- Set the slider's margins
---
--- @tparam[opt={}] table|number|nil handle_margins
--- @tparam[opt=0] number handle_margins.left
--- @tparam[opt=0] number handle_margins.right
--- @tparam[opt=0] number handle_margins.top
--- @tparam[opt=0] number handle_margins.bottom
--- @propemits true false
--- @propbeautiful
--- @method set_handle_margins
--- @noreturn
-
function slider:set_handle_margins(value)
- local value = value or 0
+ value = value or 0
+
if type(value) == "number" then
value = {
top = value,
@@ -419,19 +383,9 @@ function slider:set_handle_margins(value)
self:emit_signal("widget::layout_changed")
end
---- Set the bar's margins
---
--- @tparam[opt={}] table|number|nil bar_margins
--- @tparam[opt=0] number bar_margins.left
--- @tparam[opt=0] number bar_margins.right
--- @tparam[opt=0] number bar_margins.top
--- @tparam[opt=0] number bar_margins.bottom
--- @propemits true false
--- @propbeautiful
--- @method set_bar_margins
--- @noreturn
-
function slider:set_bar_margins(value)
+ value = value or 0
+
if type(value) == "number" then
value = {
top = value,
@@ -625,13 +579,15 @@ function slider:layout(context, width, height)
local value = self._private.value or self._private.min or 0
local min, _, interval = get_extremums(self)
- local bar_height, bar_width = self._private.bar_height or height, width
+ local bar_height = self._private.bar_height or height
local handle_height, handle_width = height, self._private.handle_width
or beautiful.slider_handle_width
or width
if bar_widget then
- bar_width = (((value - min) / interval) * width + handle_width / 2 - ((value - min) / interval ) * handle_width)
+ local bar_width = (
+ ((value - min) / interval) * width + handle_width / 2
+ - ((value - min) / interval ) * handle_width)
local w, h = base.fit_widget(self, context, bar_widget, bar_width, bar_height)
local margins = self._private.bar_margins
or beautiful.slider_bar_margins
@@ -639,9 +595,7 @@ function slider:layout(context, width, height)
local x_offset, y_offset = 0, 0
if margins then
- x_offset, y_offset = margins.left, margins.top
- bar_width = bar_width -
- (margins.left) - (margins.right)
+ x_offset, y_offset = margins.left, margins.top
end
local x, y = 0 + x_offset, 0 + y_offset
table.insert(result, base.place_widget_at(bar_widget, x, y, w, h))
@@ -725,6 +679,8 @@ end
-- @tparam[opt] number args.minimum The slider minimum value.
-- @tparam[opt] number args.maximum The slider maximum value.
local function new(args)
+ args = args or {}
+
local ret = base.make_widget(nil, nil, {
enable_properties = true,
})
@@ -735,6 +691,13 @@ local function new(args)
ret:connect_signal("button::press", mouse_press)
+ -- Initialize the tables.
+ for _, prop in ipairs {"handle_margins", "bar_margins"} do
+ if not args[prop] then
+ ret[prop] = 0
+ end
+ end
+
return ret
end
With that, the I also, there's a little helpful command called |
|
Sure |
Is there anything else you'd like me to get working before the merge? |
|
No, I need to circle back to this. But we have another PR in progress for a generc template system. It would be nice to have this merged first so we can use it in the slider. This would allow to have the bar widget to display the slider value inline (optionally) and that kind of stuff. |
|
Good to know, I'll watch for that |
I've added a
handle_widgettowibox.widget.slider. I'm not sure how to do the tests, but I'd be willing to try writing them if I was given a direction. I'm not sure on the docs, so if I could get more details on the formatting of those that'd be great!