Skip to content

Conversation

podzolelements
Copy link

@podzolelements podzolelements commented Aug 3, 2025

Fix #207 by adding a horizontal and vertical mirroring tool that operates on the actively selected elements

Basic required functionality

  • BrushStrokes
  • ShapeStrokes
  • Implement vertical mirroring
  • Make proper icons
  • Compliance with code style and pass tests

Planned: Mirroring images

  • BitmapImages
  • VectorImages

Demo

mirror_demo.mp4
mirror_images.mp4

Not currently planned

  • TextStrokes. Mirroring text seems complicated and not all that useful. If mirrored text is required, it can be pasted in and be reverse justified through the typewriter already. Perhaps this could be a "macro" that does those operations on the selected text? I might think about this more once I have more familiarity with the codebase, but only after the rest of the functionality is implemented
  • Altering the Transformable trait to include a mirror function as a basic geometric transformation. I think this would be the best way to scale the mirroring functionality to new features, however it would be a tremendous undertaking and as of writing I'm not confident I could pull it off. Again, I want to focus on getting the feature implemented and working
  • "Negative resizing" as described in Implement the ability to flip the current selection #1155. Mirroring will solve the root cause of not being able to flip the selection as mentioned in the issue, but I think negative resizing is a different enough feature to warrant a separate PR. This is sort of what I first considered doing, using only Transformable functions, but scale() would instantly crash if the x and y scalings had different signs, so I moved on from that idea

Feedback

Some things I'm not sure about yet and want to discuss:

  • Location of mirror buttons. I've currently placed it in the selection sidebar, however, Adding mirorring-feature for selected shapes. #614 has a suggestion to place the mirror tool onto the selection area itself. I personally think it would make the selection area more cluttered, especially with small selections
  • Are both a horizontal and vertical mirror tool necessary? I think so, even though geometrically a vertical mirror can be done via rotate->mirror->rotate, as that isn't super intuitive if you're not already familiar with it
  • Other ideas and thoughts are welcome for discussion

@podzolelements podzolelements marked this pull request as ready for review August 7, 2025 16:57
@Doublonmousse
Copy link
Collaborator

Not currently planned

TextStrokes. Mirroring text seems complicated and not all that useful. If mirrored text is required, it can be pasted in and be reverse justified through the typewriter already. Perhaps this could be a "macro" that does those operations on the selected text? I might think about this more once I have more familiarity with the codebase, but only after the rest of the functionality is implemented

There might be some jankiness because of this. I think it makes sense not to have it for textstrokes but maybe we need to adapt some things here.
For example if your selection contains text and other stroke elements, what should the mirror operation do ? Should we visibly deactivate/activate the buttons as soon as a selection contains/does not contain text ?

Altering the Transformable trait to include a mirror function as a basic geometric transformation. I think this would be the best way to scale the mirroring functionality to new features, however it would be a tremendous undertaking and as of writing I'm not confident I could pull it off. Again, I want to focus on getting the feature implemented and working

I think that can be done to refactor/condense this down a little later.

"Negative resizing" as described in #1155. Mirroring will solve the root cause of not being able to flip the selection as mentioned in the issue, but I think negative resizing is a different enough feature to warrant a separate PR. This is sort of what I first considered doing, using only Transformable functions, but scale() would instantly crash if the x and y scalings had different signs, so I moved on from that idea

Do you have a crash message for this ? Does it crash on a specific stroke type ? If it's relatively easy to fix maybe it's worth trying (though I'm not 100 % sure all stroke types would magically work with this)

Feedback

Location of mirror buttons. I've currently placed it in the selection sidebar, however, #614 has a suggestion to place the mirror tool onto the selection area itself. I personally think it would make the selection area more cluttered, especially with small selections

I agree with this. I think it makes more sense to do negative resizing than adding the mirror tool on the selection.

One additional thing is that the selector panel is starting to get a little crowded. Two more icons, and maybe an additional one down the line with #1428. One button I'm using a lot in the selector is the delete button and I'm not sure it won't get out of bounds with this. Maybe we're gonna have to reorder things a little bit, add separators, or submenus ?

@podzolelements
Copy link
Author

For example if your selection contains text and other stroke elements, what should the mirror operation do ?

Currently it can make kind of a mess, which I think needs to be addressed, especially when you would be expecting the text to react to the mirror.

Should we visibly deactivate/activate the buttons as soon as a selection contains/does not contain text ?

I think that might be the simplest option, which might be a little annoying from a user perspective, though probably less so than the current implementation of leaving the text completely alone. I've thought about it a bit more since then, and I think mirroring characters is a really bad idea. What are your thoughts on 'positionally mirroring' text, where the position gets updated with the rest of the selection, and something like the following happens:
text_reversed

Though the justification would probably still need to be reversed, as in the following:
rejustify

Not to mention for vertical mirrors the line order would need to be reversed:
text_order_reversal
I'd be down to explore this avenue if you think it is reasonable, and we could always fall back on disabling the buttons with text selections in them if it turns out to be a disaster/introduce a bunch of other issues (or come up with other ideas).

One additional thing is that the selector panel is starting to get a little crowded.
Maybe we're gonna have to reorder things a little bit, add separators, or submenus ?

Maybe the 4 selection types could be put into a selection menu like how the shape selector currently works? I personally find I don't change selection modes much of at all, but I don't know about other people's workflows. Also the 'lock aspect ratio' button could go in there as well, freeing another space, which would reduce the current items from 12 to 8, something like:
selection_menu_idea
Or it could be more like the brush configuration menu, and have the descriptions built in vs as mouseover text?
Perhaps the 'select all' and 'deselect all' strokes could be condensed into more of a single toggle button? Just throwing some ideas out there.

@Doublonmousse
Copy link
Collaborator

Doublonmousse commented Aug 24, 2025

Very good ! I've tested exporting as well just in case to all formats and this works too.

  • Should we add accelerations/keyboard shortcuts for the actions ?
  • Should we do something for text ? Special mode or deactivate the buttons/actions if text is involved ?
  • Do we start doing something for buttons now (or we keep it as a follow up ?).

I'm adding @flxzt to discuss the remaining questions.

@Doublonmousse Doublonmousse requested a review from flxzt August 24, 2025 16:35
@podzolelements
Copy link
Author

Should we add accelerations/keyboard shortcuts for the actions ?

Good idea, what do we think for keys? h and v would probably be best, though I personally would have them as x and y.

Should we do something for text ? Special mode or deactivate the buttons/actions if text is involved ?

I'll experiment with text mirroring with the functionality as described in my recent reply.

Do we start doing something for buttons now (or we keep it as a follow up ?).

I think it makes most sense to reorganize the selection menu as a separate PR, as that could include other fixes that significantly differ functionally from mirroring, most notably #155. I'd be interested in working on it right after this though.

Copy link
Collaborator

@Doublonmousse Doublonmousse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's only a minor fix to do for gettext here. After the fix I'll run the CI one more time (to check/listen to clippy)

At some point I imagined toggling the button's sensitivity in the toolbar based on context (on each selection update) so you wouldn't be able to click as soon as a text element would be in the selection. Haven't totally though out the mechanism though ...

But I'm pretty satisfied with the popup method so all is good.

@podzolelements
Copy link
Author

so you wouldn't be able to click as soon as a text element would be in the selection. Haven't totally though out the mechanism though ...

Yeah I wasn't sure how to do this on the UI side so I went with the popup.

@podzolelements
Copy link
Author

podzolelements commented Aug 27, 2025

There seems to be some odd behavior when running the visual debug mode, the bounding boxes don't update their rendering properly. Affects both horizontal and vertical mirrors, and when the selection is dragged it updates properly. Not sure if there is something I'm missing in WidgetFlags or where to look if this is an issue with how the visual debug mode works.

odd_debug_mirror

Edit: Actually this seems to be consistent with how the visual debugger currently works. Dragging around and not releasing a selection has the same behavior, where the bounding boxes stay behind until the selection is released.

Copy link
Collaborator

@Doublonmousse Doublonmousse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visual debug hitboxes only updating on a pen up event is an optimization : do not regenerate the hitboxes when we are only moving the strokes. As there are primarly used by the different selector to determine what strokes are selected, and we can't create a new selection in the middle of dragging an element, it makes sense to defer the computation to the pen up event.

Not updating the stroke geometry breaks the 4th Select Intersecting Path selection mode here
Screencast_20250827_200034.webm

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.

Mirror selection
2 participants