Skip to content

Create cursor_tool object and provider. #117

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

haske6155
Copy link

Please enter a short description of your pull request and add a reference to the Jira ticket.

New Features and Enhancements

  • New Features and Enhancements

Refactorings and Bug Fixes

  • Refactorings and Bug Fixes

Checklist

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

  • Include the name of the Jira ticket in the PR’s title
  • Add the link to the ticket in Jira in the description of the PR
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines (Wiki)
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Add new information to the Wiki

@juliajulie95 juliajulie95 marked this pull request as draft May 15, 2025 11:21
@haske6155 haske6155 marked this pull request as ready for review July 6, 2025 14:50
Copy link

@Copilot Copilot AI left a 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 PR introduces a new CursorTool, its provider, integrates it into the toolbox state and painter, and adds corresponding unit & integration tests, plus minor UI painter and iOS project config updates.

  • Added CursorTool implementation and provider, and wired it into ToolBoxStateProvider & CommandPainter
  • Added comprehensive unit tests (cursor_tool_test.dart) and integration tests (cursor_tool_test.dart)
  • Updated CanvasPainter to render the cursor icon and adjusted iOS scheme/project for GPU validation and pod resource copying

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/unit/tools/cursor_tool_test.dart Added unit tests covering cursor positioning and drawing behavior
test/integration/cursor_tool_test.dart Added integration tests for cursor tool interactions
lib/ui/pages/workspace_page/components/drawing_surface/canvas_painter.dart Render cursor icon instead of const Stack
lib/core/tools/tool.dart Changed Tool constructor from const to non-const
lib/core/tools/implementation/cursor_tool.dart New CursorTool implementation
lib/core/providers/state/toolbox_state_provider.dart Integrated ToolType.CURSOR into toolbox state
lib/core/providers/object/tools/cursor_tool_provider.dart Added Riverpod provider for CursorTool
lib/core/commands/command_painter.dart Draw cursor icon in the painter switch
ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme Enabled GPU validation
ios/Runner.xcodeproj/project.pbxproj Added pod resource copy build phase
Comments suppressed due to low confidence (4)

test/integration/cursor_tool_test.dart:50

  • The test block for testID 1 and the test block for testID 2 are identical. Consider removing one of the duplicate tests to reduce redundancy and speed up CI.
  if (testID == -1 || testID == 1) {

lib/ui/pages/workspace_page/components/drawing_surface/canvas_painter.dart:24

  • [nitpick] Removing the const qualifier from the Stack prevents compile-time optimizations. If the children are immutable, consider restoring const Stack to improve rendering performance.
      child: Stack(

lib/core/tools/tool.dart:15

  • [nitpick] The constructor was changed from const Tool to non-const; since all fields are final, you could reintroduce const here to allow compile-time instantiation and improve code clarity.
  Tool({

ios/Runner.xcodeproj/project.pbxproj:272

  • This appears to add a duplicate '[CP] Copy Pods Resources' build phase, which can lead to duplicated resource copies or build confusion. Consider removing or merging with the existing phase.
		CE347BAC542BC77A1AF7E630 /* [CP] Copy Pods Resources */ = {

@@ -77,6 +78,9 @@ class ToolBoxStateProvider extends _$ToolBoxStateProvider {
(state.currentTool as SprayTool).updateSprayRadius(currentStrokeWidth);
ref.read(paintProvider.notifier).updateStrokeWidth(SPRAY_TOOL_RADIUS);
break;
case ToolType.CURSOR:
state = state.copyWith(currentTool: ref.read(cursorToolProvider));
Copy link
Preview

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

When selecting the cursor tool, the paint stroke width isn’t updated. You may need to call ref.read(paintProvider.notifier).updateStrokeWidth(...) with the cursor’s stroke width to ensure drawing uses the correct thickness.

Suggested change
state = state.copyWith(currentTool: ref.read(cursorToolProvider));
state = state.copyWith(currentTool: ref.read(cursorToolProvider));
ref.read(paintProvider.notifier).updateStrokeWidth(1.0); // Default stroke width for cursor

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@Lenkomotive Lenkomotive left a comment

Choose a reason for hiding this comment

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

Overall good work:

  1. pls check out the redo/undo function and how it works in other tools. I noticed some inconsistency and also redoing to a line which was drawn by a cursor should also switch back to cursor tool. Might need to look at the PathCommand.
  2. pls use the Catrobat PR-template and check if you have ticked all the boxes.

@@ -21,7 +21,7 @@ class CanvasPainter extends ConsumerWidget {
foregroundDecoration: const BoxDecoration(
border: Border.fromBorderSide(BorderSide(width: 0.5)),
),
child: const Stack(
child: Stack(
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove const?

@@ -11,7 +12,7 @@ abstract class Tool {
final bool hasAddFunctionality;
final bool hasFinalizeFunctionality;

const Tool({
Tool({
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove const?

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.

2 participants