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.

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

Add compass #4443

Merged
merged 4 commits into from
Jan 1, 2023
Merged

Add compass #4443

merged 4 commits into from
Jan 1, 2023

Conversation

rolandlo
Copy link
Member

@rolandlo rolandlo commented Nov 24, 2022

This PR is split off from (and based on) #4211. Only the last commit 96354a9 is relevantcommits from "Add compass" are relevant.

Here is a little demo (mostly using the keyboard; m is for marking the center):

CompassDemo.mp4

Edit: Added demo (Nov 27)

@rolandlo rolandlo force-pushed the feature-compass branch 4 times, most recently from d300be5 to cda7287 Compare November 26, 2022 16:25
@rolandlo rolandlo changed the title WIP: Add compass Add compass Nov 27, 2022
@rolandlo
Copy link
Member Author

After #4211 has been reviewed and merged, this PR may be ready for review.

@rolandlo rolandlo force-pushed the feature-compass branch 4 times, most recently from 268bead to 18376c2 Compare December 7, 2022 06:14
@rolandlo rolandlo force-pushed the feature-compass branch 2 times, most recently from 73e2f3c to 1f02dff Compare December 17, 2022 11:11
@rolandlo
Copy link
Member Author

Rebased on master after merging the setsquare refactoring. This is ready for review.

@rolandlo rolandlo force-pushed the feature-compass branch 2 times, most recently from 4a1a176 to bfe1bd9 Compare December 17, 2022 11:28
Copy link
Member

@Technius Technius left a comment

Choose a reason for hiding this comment

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

I skimmed through the code quickly, some comments below.

src/core/view/CompassView.h Outdated Show resolved Hide resolved
src/core/control/Control.cpp Outdated Show resolved Hide resolved
src/core/view/CompassView.cpp Outdated Show resolved Hide resolved
src/core/view/CompassView.cpp Outdated Show resolved Hide resolved
src/core/view/CompassView.cpp Outdated Show resolved Hide resolved
src/core/view/CompassView.cpp Show resolved Hide resolved
src/core/control/Control.cpp Outdated Show resolved Hide resolved
src/core/control/Control.cpp Outdated Show resolved Hide resolved
@rolandlo
Copy link
Member Author

Merging in 48 hours if no objections are raised.

@rolandlo rolandlo added the merge proposed Merge was proposed by maintainer label Dec 30, 2022
@Technius
Copy link
Member

Technius commented Jan 1, 2023

I played around with this briefly and noticed a few things:

  • This uses the same controls as the setsquare, so that would be nice to note in Add geometry tools documentation xournalpp.github.io#77
  • When drawing a line from the center, the line can only extend in length. If the user overshoots, they have to undo the stroke and try again.
  • For the center drawing feature, I think it would be better to have a feature where the user can drag from the center to the edge to fix an angle, and then move along the pending line to adjust the radius. This can be left to a future PR, however.
  • The following bug affects both geometry tools and can be addressed in a follow up PR: only one arrow key direction can be active at a time. E.g., when pressing both up+right, the geometry tool only moves in the direction of the first arrow key pressed.

These are not major showstoppers, so they can be addressed in a follow up.

@rolandlo rolandlo merged commit ebc889e into xournalpp:master Jan 1, 2023
@rolandlo rolandlo deleted the feature-compass branch January 1, 2023 06:39
@rolandlo
Copy link
Member Author

rolandlo commented Jan 1, 2023

Thanks for the feedback.

I played around with this briefly and noticed a few things:

* This uses the same controls as the setsquare, so that would be nice to note in [add setsquare documentation xournalpp.github.io#77](https://github.com/xournalpp/xournalpp.github.io/pull/77)

I'm working on it.

* When drawing a line from the center, the line can only extend in length. If the user overshoots, they have to undo the stroke and try again.

It's the same for drawing on the outline of the compass and for drawing on the longest side of the setsquare or drawing on physical paper.

* For the center drawing feature, I think it would be better to have a feature where the user can drag from the center to the edge to fix an angle, and then move along the pending line to adjust the radius. This can be left to a future PR, however.

Interesting idea. It wouldn't allow drawing circular ring (sectors) anymore though.

* The following bug affects both geometry tools and can be addressed in a follow up PR: only one arrow key direction can be active at a time. E.g., when pressing both up+right, the geometry tool only moves in the direction of the first arrow key pressed.

Is that also a bug of moving the page around (without geometry tools then)? Haven't thought about having multiple arrow keys at the same time.

These are not major showstoppers, so they can be addressed in a follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge proposed Merge was proposed by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants