-
-
Notifications
You must be signed in to change notification settings - Fork 22
Add Smart Lasso Selection feature #158
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
base: main
Are you sure you want to change the base?
Conversation
|
Let copilot review it first, maybe it will find some problems. I will look at it latter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds a "smart lasso" selection feature that allows users to draw a closed loop to select content. When enabled, users can dismiss the selection to fall back to drawing the original stroke, providing a more intuitive drawing/selection workflow.
- Adds a new
smartLassoEnabledsetting to app configuration - Implements smart lasso selection detection in drawing logic
- Adds fallback mechanism to draw original stroke when selection is dismissed
- Enhances JSON deserialization for backward compatibility
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/strings.xml | Added English string resource for smart lasso setting |
| app/src/main/res/values-pl/strings.xml | Added Polish translation for smart lasso setting |
| app/src/main/java/com/ethran/notable/ui/views/Settings.kt | Added UI toggle for smart lasso feature in settings |
| app/src/main/java/com/ethran/notable/editor/ui/SelectorBitmap.kt | Refactored selection dismissal to use centralized control tower method |
| app/src/main/java/com/ethran/notable/editor/state/SelectionState.kt | Added state tracking for pending smart lasso strokes |
| app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt | Implemented dismissSelection method with smart lasso fallback logic |
| app/src/main/java/com/ethran/notable/editor/DrawCanvas.kt | Integrated smart lasso detection in drawing input handling |
| app/src/main/java/com/ethran/notable/data/db/Kv.kt | Added JSON configuration for backward compatibility |
| app/src/main/java/com/ethran/notable/data/datastore/AppSettings.kt | Added smartLassoEnabled field to app settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt
Outdated
Show resolved
Hide resolved
|
@Ethrlet you wanna have a look now? |
|
I will look into it this week, I need some more time to do it. |
Ethran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some unnecessary operations in your implementation, please be mindful of what you are calculating before drawing strokes on the screen.
History should handle uncommitted strokes.
Don't duplicate existing functions, if something is implemented already, use it.
The EditorControlTower will change a little bit, mind this doing changes. 57d5fe2
| Log.i("SmartLasso", "User dismissed smart lasso selection, drawing the original stroke with original pen settings") | ||
| // User dismissed without using the panel, so draw the original stroke with original settings | ||
| // Save the pending data before reset clears it | ||
| val strokeToDrawCopy = pendingStroke.toList() | ||
| val penCopy = pendingPen | ||
| val strokeSizeCopy = pendingStrokeSize | ||
| val colorCopy = pendingColor | ||
|
|
||
| // Reset the selection | ||
| state.selectionState.reset() | ||
|
|
||
| // Now draw the pending stroke with its original pen settings | ||
| val strokeHistoryBatch = mutableListOf<String>() | ||
| com.ethran.notable.editor.utils.handleDraw( | ||
| page, | ||
| strokeHistoryBatch, | ||
| strokeSizeCopy, | ||
| colorCopy, | ||
| penCopy, | ||
| strokeToDrawCopy | ||
| ) | ||
|
|
||
| // Add to history | ||
| if (strokeHistoryBatch.isNotEmpty()) { | ||
| history.addOperationsToHistory( | ||
| operations = listOf( | ||
| Operation.DeleteStroke(strokeHistoryBatch) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| state.isDrawing = true | ||
|
|
||
| // Refresh UI | ||
| scope.launch { | ||
| DrawCanvas.refreshUi.emit(Unit) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be moved to separate funtion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it should be done as new operation list in History(pageView: PageView),
private var uncommittedOperations: OperationList = mutableListOf()And there should be functions:
commitOperations()
addToUncommittedOperations(…)
clearUncommittedOperations(…)
(There probably are better names for them)
See also branch dev, I made recently some changes how the history is handled (I needed to handled one thing differently, and ended up changing a lot):
Commit: 57d5fe2
It will be merged when I address most of TODO's in this marge request. #156
| private val kvRepository = KvRepository(context) | ||
|
|
||
| // Configure JSON to ignore unknown keys for backward compatibility | ||
| private val json = Json { ignoreUnknownKeys = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to check I really want to enable it. Having it set to false makes sure that I know exact state of app config. For now this PR should not make this change.
| private fun distance(p1: StrokePoint, p2: StrokePoint): Float { | ||
| val dx = p1.x - p2.x | ||
| val dy = p1.y - p2.y | ||
| return sqrt(dx * dx + dy * dy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use sqrt. It adds unnecessary computations, and its enough to use Manhattan metric. See scribble to erase.
| val boxHeight = boundingBox.height() | ||
|
|
||
| // Calculate diagonal of bounding box | ||
| val diagonal = sqrt(boxWidth * boxWidth + boxHeight * boxHeight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, use Manhattan metric.
| /** | ||
| * Calculates the Euclidean distance between two points | ||
| */ | ||
| private fun distance(p1: StrokePoint, p2: StrokePoint): Float { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as distance might will be used in multiple parts of project it should be moved to editor/utils/…
| /** | ||
| * Calculates the total perimeter length of a stroke path | ||
| */ | ||
| private fun calculatePerimeter(points: List<StrokePoint>): Float { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its just a path length, not a Perimeter. Also, reuse function from scribble to erase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it seems as a quite bad way to determine if it is the loop. I would propose to select some points on the path, and check if distance between them is sufficiently large. ie let those points divide path in roughly 4 parts, and calculate distance between those 3 points. BUT be careful not to do too many operations, check if choosing base on index in table is good enough, or base on time (binary search).
| * Calculates direction reversals but with different thresholds for lasso detection | ||
| * Unlike scribble detection, we're looking for smooth loops, not back-and-forth motion | ||
| */ | ||
| private fun calculateNumReversalsForLasso( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it is separate function from what is used for scribble to ease?
| editorState.selectionState.pendingSmartLassoStroke = touchPoints | ||
| editorState.selectionState.pendingSmartLassoPen = editorState.pen | ||
| editorState.selectionState.pendingSmartLassoStrokeSize = editorState.penSettings[editorState.pen.penName]?.strokeSize | ||
| editorState.selectionState.pendingSmartLassoColor = editorState.penSettings[editorState.pen.penName]?.color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't good way of storing this information. It should be stored the same as stack for undo/redo.
| // Smart lasso: stores the original stroke data if selection was made via smart lasso | ||
| // This allows fallback to drawing the stroke if user dismisses without using the panel | ||
| var pendingSmartLassoStroke by mutableStateOf<List<StrokePoint>?>(null) | ||
| var pendingSmartLassoPen by mutableStateOf<Pen?>(null) | ||
| var pendingSmartLassoStrokeSize by mutableStateOf<Float?>(null) | ||
| var pendingSmartLassoColor by mutableStateOf<Int?>(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the place for those, see other comments.
| // Additional check: ensure the stroke doesn't have too many sharp reversals | ||
| // (which would indicate scribbling rather than deliberate loop drawing) | ||
| // We allow some reversals (unlike scribble which requires many), but not too many | ||
| val reversals = calculateNumReversalsForLasso(points) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to calculate it again? We already calculate it in scribble to erase, if both are turned on, then we do unnecessary calculations. Same for path lengths.
|
Hi @Ethran Life's gotten busy, I'm afraid. I've also stopped using Notable for the most part recently (no major reason). |
This is based on discussion in #140.
Summary
This PR adds a Smart Lasso Selection feature that allows users to select content by drawing a closed loop with their pen/stylus, making selection much more intuitive for EMR device users. The feature intelligently detects closed loops and shows the selection panel, with smart fallback to drawing if the user dismisses without taking action.
How does this work?
Note: Scribble-to-erase takes priority, so it doesn't interfere with normal drawing.
User Flow
a) Use tool (cut/copy/paste/duplicate) → Content is selected/manipulated
b) Tap outside → Original loop stroke is drawn on page
Closed loop detection algorithm
It uses several heuristics to ensure accurate recognition:
Here's a video showcasing the feature and settings: https://drive.google.com/file/d/17nynCt7uNkB5glhnBmw1N1AGgTBBcnRe/view?usp=sharing
Maybe in Future?
Allow tuning detection thresholds in advanced settings