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.

Sign up for GitHub

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

Jump to bottom

Closed Polyline path #1863

Merged
merged 8 commits into from
Jul 24, 2024
Merged

Closed Polyline path #1863

merged 8 commits into from
Jul 24, 2024

Conversation

HeCorr
Copy link
Contributor

@HeCorr HeCorr commented Jul 21, 2024 •
edited
Loading

This PR adds the Closed option to the Polyline tool which automatically closes it's path as it's being drawn.

The Ctrl key can be held to temporarily override the closed state:

  • If the Closed option is on, holding Ctrl temporarily disables it
  • If the Closed option is off, holding Ctrl temporarily enables it

Video

2024-07-22.21-42-20_edit.mp4

It works with both double-clicking and pressing Enter to apply the Polyline.

@HeCorr
Copy link
Contributor Author

HeCorr commented Jul 22, 2024

This has been discussed on Discord and I decided to add a tool option for this which inverts the behavior of the Ctrl key, so that:

  • If the Closed option is on, holding Ctrl temporarily disables it
  • If the Closed option is off, holding Ctrl temporarily enables it

So that users who prefer to use the Ctrl key can leave the option off and those who don't can leave the option on with the added benefit of temporarily disabling it if necessary without having to toggle the tool option.

@HeCorr
rename mClosed to mClosedOverride
78d4d44
@HeCorr
add Closed tool option to Polyline tool
327ccbc 
it makes it so the Polyline path can be automatically closed, and the Ctrl key now temporarily inverts the behavior while held.
@HeCorr HeCorr changed the title Hold Ctrl key to close Polyline path Closed Polyline path Jul 23, 2024
@HeCorr HeCorr marked this pull request as ready for review July 23, 2024 01:32
MrStevns
MrStevns requested changes Jul 23, 2024
View reviewed changes
Copy link
Member

@MrStevns MrStevns left a comment

Choose a reason for hiding this comment

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

Functionality wise I have no issues with this PR. There are however some minor things I'd like addressed before we merge this.

MrStevns
MrStevns reviewed Jul 23, 2024
View reviewed changes
app/ui/tooloptions.ui Outdated Show resolved Hide resolved
@HeCorr
requested changes (Polyline closed path)
1a7401a 
- `_closed` -> `closed`
- `setClosed()` -> `setClosedPath()`
- `mClosedOverride` -> `mClosedPathOverrideEnabled`
- `Closed` -> `Closed Path`
@HeCorr
Copy link
Contributor Author

HeCorr commented Jul 23, 2024

The requested changes have been committed!

@HeCorr HeCorr requested a review from MrStevns July 23, 2024 20:15
MrStevns
MrStevns approved these changes Jul 24, 2024
View reviewed changes
Copy link
Member

@MrStevns MrStevns left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I took the PR for another spin and noticed a few other things I forgot to check the last time, sorry about that.

Things that wasn't working that I've fixed for you:

  • The check state would not be saved to our settings file, meaning settings.value("closedPolylinePath").toBool() would always return false.
  • The checkbox in tooloptionswidget would not be updated on startup.

I also took the liberty to rename useClosedBox to useClosedPathBox.

With those things taken care of, this PR should imo. be good to go.
Feel free to merge the PR if you're satisfied with those changes.

@HeCorr
Copy link
Contributor Author

HeCorr commented Jul 24, 2024

The check state would not be saved to our settings file

I did notice the state wasn't being saved, but then I tested the Bézier option and it didn't save either hahah

Thanks for the fixes though!

@MrStevns
Copy link
Member

Yeah it's not consistent... ideally all tool option properties should be saved to settings.

btw. are you allowed to merge the PR yourself?

@HeCorr
Copy link
Contributor Author

HeCorr commented Jul 24, 2024

are you allowed to merge the PR yourself?

nope
Screenshot_2024-07-24-15-15-40-988

@MrStevns MrStevns merged commit 92c2281 into pencil2d:master Jul 24, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrStevns MrStevns MrStevns approved these changes

Assignees
No one assigned
Labels
Enhancement Polyline Tool
Projects
Pull Request Priority
Status: Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HeCorr @MrStevns @J5lx