feat: new options for the vertical space tool

[Doublonmousse]

Adds the following options to the vertical space bar

For #1048 and #878

• [x] make the two last options work
• [x] adapt the gui part of the tool to only go as far as the page widths selected

[Doublonmousse]

I think I've done everything except doing the more complex tool with borders that can be edited. I wonder if it's not better to do this later in a separate PR.

[flxzt]

I like this a lot from a functionality POV.

Some suggestions for improvements in the UI: I don't like the location of the added options. It would be better to place this right next to the tool and rearrange the tools sidebar with visible separators between the individual tools. This would also be consistent to the other sidebar pages.

There are also some minor inconsistencies like the option button size which I think should be the smaller variant.

Also: the option names could improved imho. What about instead of "Shape Configuration" -> " "Vertical-Space Tool Options", "Respect Vertical/Horizontal Borders" -> "Limit movement to Vertical/Horizontal Page Borders" ?

[Doublonmousse]

I've went for the linked style of other toolspages for now.. I think it's a little better at showing that there are three different tools than before.

It doesn't really fix the fact that the settings icon isn't related to the vertical space tool though. Ideally I'd like to keep the linked aspect of the three tools whilst making the settings icon appear tied to the vertical space tool

[flxzt]

What do you think about something like this?

with the additional logic that the vertical-space tool is automatically selected when clicking on it's options

(see this branch: https://github.com/flxzt/rnote/tree/flxzt/vertical-tools-more-improv)

[Doublonmousse]

Yes, that works well !

[flxzt]

alright, I'll add my commit then.

Oh and I am not sure the force snapping option is actually needed, since it is already covered by the "snap positions" option. It could be confusing when deactivating one of them, why it seemingly is not respected

[Doublonmousse]

Yeah, makes sense. It's more of a habit I took since https://github.com/flxzt/rnote/discussions/721

I think it makes sense to have a second snap mode where displacement are multiple of the grid size as opposed to snapping elements to the grid (usecase : select some text and change its position so that it stays aligned to the grid). But, same thing as before, it makes more sense to do that as a global setting than this

[flxzt]

Yeah. I am a bit concerned for that option because of feature creep of more "niche" use cases.

Can be a follow-up?

[Doublonmousse]

Yes, I agree it makes sense to remove the "force snapping" option from the current PR.

And I'll see if I want to do a follow up for snapping options later (but there is probably less niche things to add to snapping before I do that)

[Doublonmousse]

The CI check is giving warnings for a line that doesn't exist ?

 warning: unused import: `tracing::error`
  --> crates/rnote-engine/src/store/stroke_comp.rs:15:5
   |
15 | use tracing::error;
   |     ^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

But there's nothing https://github.com/Doublonmousse/rnote/blob/vertical_tool/crates/rnote-engine/src/store/stroke_comp.rs#L15

(I'm not getting the error on rustc 1.80.0 so it's probably a compiler bug)

[flxzt]

I noticed it as well. Seems to be a false positive lint.

One additional point, which I am not sure about:

Should strokes that only intersect the limits be included as well or not?

[Doublonmousse]

I think it's better to have all selection tools work when the full stroke is in bounds, and add a global option for partial selection like #155 for all tools.

Do you think adding a graphical indicator when limiting the selection horizontally to show where the selection stops is a good idea ? I also wondered if I should do something in that case to make it harder to accidentally overlap strokes onto the next page (like some snapping or indicator when the selection would touch the next horizontal border)

[flxzt]

At least from my testing it was intuitive enough without an indicator.

Snapping sounds like a good idea, I can see it being useful often

[Doublonmousse]

I'm not sure snapping is a super good idea. I don't know how to deal with potentially two snapping modes at the same time (General snapping to grid + the added one when crossing the border)

Maybe it makes more sense to limit the amplitude of the tool so that the selected portion stays in the same page instead. This way I don't have the issue with multiple snapping modes, and we don't end up with strokes that become unmovable by the vertical space tool when using the limiter on horizontal borders (because of the decision to only include strokes that are fully in bounds).

[flxzt]

Couldn't there be both? Just like content already also snaps to the page borders when Snap positions is enabled.

[flxzt]

But implementing snapping is also not a blocker for me

[Doublonmousse]

Oh I see, that's indeed possible to have two snapping modes.

That would make placing strokes between pages when using the horizontal limit less of an issue.

Maybe that can be a follow up ? I wonder if it's better to select strokes that intersect the bounds in this PR in the meantime

[flxzt]

Maybe that can be a follow up ?

Of course.

I wonder if it's better to select strokes that intersect the bounds in this PR in the meantime

I just tested it again, and including intersecting strokes feels more intuitive, so I'd say yes.

But I found one remaining issue: When both limits are enabled in the UI, the vertical option actually is not respected. There seems to be a logic error in the implementation

[Doublonmousse]

I think it's working now

[flxzt]

Looks good now, nice!