Improve Split accuracy. by xStrom · Pull Request #738 · linebender/druid

xStrom · 2020-03-22T18:14:14Z

This PR includes a slew of changes to the Split widget.

All layout and drawing is now quantized to integers as a continuation of work in Align Container and debug borders to pixels. #669. This means that single pixel lines never bleed out to side pixels. This applies to both the inner children and the split bar.
Adds min_splitter_area(f64) method which basically acts as padding around the split bar. This padding still reacts to mouse events. As a result it is possible to use extremely narrow split bars while still having a reasonable area for the user to click on. Fixes The handle of the Split widget should have a minimum hit area #641.
No longer forgets the split_point chosen by the user. The widget still modifies the split point when the size constraints require it (e.g. when resizing the window to extremely small) however now the user chosen split point is restored as soon as the constraints allow it.
The meaning of splitter_size has slightly changed, to a less surprising interpretation.
- Previously a splitter size of 10 with a solid fill meant:
  4px wide solid fill with 3px padding on each side.
  With the fill width formula being: size - 2*((size/3).floor())
  Now a 10 with a solid fill means a 10px wide solid fill.
  The new min_splitter_area can be used to add padding.
- Previously a splitter size of 10 with a line style meant:
  2px pad | ~2px line | 2px pad | ~2px line | 2px pad
  With 1px lines being centered (size/3).floor() pixels away from edges.
  Now a 10 with a line style means: 3px line | 4px pad | 3px line
  Padding at edges can be added optionally with min_splitter_area.
Renamed a few internal functions and added a few lines of documentation for better readability.

Generally speaking the Split widget still has issues, which were there before and aren't addressed here, e.g. children still being drawn when they have zero sized layout. I'll probably tackle this at least partially in a future PR.

cmyr · 2020-03-23T15:40:42Z

@Zarenor are you around? Any interest in taking a look at this?

cmyr

Okay a couple little notes, but looks good. Sorry for the wait.

druid/src/widget/split.rs

cmyr · 2020-04-02T14:05:50Z

druid/src/widget/split.rs

    }
-    fn splitter_hit_test(&self, size: Size, mouse_pos: Point) -> bool {
+
+    /// Returns the width of the area of mouse hit detection.


'width' is kind of confusing for a vertical splitter, but whatever 🤷‍♂

'breadth', so we're not implying directionality (vs 'width' or 'height')?

I'd used 'size' before, because I hadn't been able to come up with 'breadth'

I think breadth is probably more confusing than advantageous. I think size is good?

Yeah the usage of width is a bit confusing. Previously Split used both size and width to refer to this and I didn't spend much time thinking about it. I think it's best to use only one word for this concept and I'll do that update.

As for what word to use, size is decent. I can also think of thickness and cross length. I kind of like thickness.

whichever you like; I still prefer size I think, thickness has z-axis implications for me?

Well there's no z-axis involved with Split so that's an unlikely clash. Also Flutter seems to use thickness.

That said, I went with size for now due to its shortness and better fit with area.

cmyr · 2020-04-02T14:07:40Z

druid/src/widget/split.rs

+    split_point_chosen: f64,
+    split_point_effective: f64,
    splitter_size: f64,
+    min_splitter_area: f64,


Maybe this should be an Option<f64>, so that we can default to using the splitter size when this isn't passed explicitly? Maybe?

I'm not sure. I'm just thinking of in the example where you set the splitter size to 3, should we keep the same default min_splitter_area? Maybe...

I think having it set by default with some reasonable value is best. The main reason this exists is to allow extremely narrow splitter bars without sacrificing the user experience by requiring them to pixel hunt. So if someone wants a 3px bar, or even a 1px bar, they can easily do so but druid will default to a 6px bar area. If they want the area to also be 1px then they can set the minimum area to e.g. 0px.

druid/src/widget/split.rs

Zarenor · 2020-04-02T15:02:31Z

druid/src/widget/split.rs


-        let min_offset = (self.splitter_size * 0.5).min(5.0);
-        let mut min_limit = self.min_size.max(min_offset);
-        let mut max_limit = (size_in_splitted_direction - self.min_size.max(min_offset)).max(0.0);
+        let mut min_limit = self.min_size;
+        let mut max_limit = (size_in_split_direction - min_limit).max(0.0);


I see we've removed the absolute '5.0' minimum. This was originally added to 'fix' the possibility that the splitter would get 'lost' in the edge of it's container - or that nested splitters could collide.
I'm okay with removing it if the behavior is still acceptable in these extrema, but it's something worth checking for.

This minimum isn't needed with the new logic, because the splitter bar is guaranteed to always be visible. This specific function calculates the min/max limits given the provided size and doesn't need to know about the splitter bar at all. The bar logic is elsewhere.

Zarenor · 2020-04-02T15:08:47Z

I'm sorry I didn't see this earlier. I added a few more comments here.

I'm really glad to see the work on quantizing move forward, and I appreciate the other improvements made.

Looks great!

xStrom · 2020-04-05T17:57:30Z

I addressed the feedback and made some other improvements.

Improved documentation, including unifying language for builder methods.
Renamed a bunch of variables and methods for consistency and reducing confusion, e.g.:
- What was known previously as splitter is now the splitter bar to make it clear it's different from Split which is the whole widget. So splitter bar is how it's referenced in documentation, but actual methods are just bar for shortness.
- The width/breadth/thickness of the splitter bar is now always called size.
- fill_splitter_handle to solid_bar to use the new bar nomenclature and to make it more concise.
- split_direction is now split_axis and this has some functional changes too, which I describe below.
Swapped the meaning of the Split::new(axis), Split::horizontal(), and Split::vertical() constructors. Previously it meant that the splitter bar is horizontal/vertical, but now it means that the horizontal/vertical axis is split in half. I think this reads better and this also matches what other frameworks are doing, e.g. Qt, GTK, JavaFX, and Metro Components.
The effective split point calculation now properly takes the bar area into account.
The minimum split child size, bar size, and bar area is now rounded up to integers.
Fixed the check for infinite height.
Reduced code size a bit by some refactoring.

xStrom · 2020-04-08T12:40:51Z

Based on the discussion in zulip I renamed the constructor helpers from vertical/horizontal to row/cols.

Split::columns(child1, child2) // left and right
Split::rows(child1, child2)    // top and bottom

cmyr

okay, works for me, I think making the names consistent with Flex is a good solution.

xStrom added the S-needs-review waits for review label Mar 22, 2020

cmyr approved these changes Apr 2, 2020

View reviewed changes

Zarenor reviewed Apr 2, 2020

View reviewed changes

druid/src/widget/split.rs Outdated Show resolved Hide resolved

Zarenor reviewed Apr 2, 2020

View reviewed changes

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 3, 2020

xStrom added 2 commits April 5, 2020 20:32

Improve Split accuracy.

cbffed3

Improve Split correctness, method naming, and documentation.

37b2538

xStrom force-pushed the splitter-qint branch from ba7e279 to 37b2538 Compare April 5, 2020 17:35

xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 5, 2020

Rename Split horizontal/vertical constructors to columns/rows.

ff704ed

cmyr approved these changes Apr 8, 2020

View reviewed changes

xStrom merged commit b9748fc into linebender:master Apr 8, 2020

xStrom removed the S-needs-review waits for review label May 15, 2020

Conversation

xStrom commented Mar 22, 2020

Uh oh!

cmyr commented Mar 23, 2020

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zarenor commented Apr 2, 2020

Uh oh!

xStrom commented Apr 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xStrom commented Apr 8, 2020

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xStrom commented Apr 5, 2020 •

edited

Loading