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

PAINTROID-759: Add Text Tool #86

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

PAINTROID-759: Add Text Tool #86

wants to merge 10 commits into from

Conversation

bhav-khurana
Copy link
Contributor

@bhav-khurana bhav-khurana commented Jun 30, 2024 •
edited by jira bot
Loading

IN DEVELOPMENT

Ticket

PAINTROID-759

New Features and Enhancements

Adds the text tool feature as mentioned in the ticket.

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)
  • Make sure to organize imports with 'make sort' before committing
  • 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

@bhav-khurana bhav-khurana marked this pull request as ready for review July 7, 2024 15:10
@bhav-khurana
Copy link
Contributor Author

Hi @bakicelebi @msesko @juliajulie95
I have tried adding a basic text tool functionality in the application but I am facing a bit difficulty in debugging some things, can you please check this and help me understand why is the drag in the text field container misbehaving?

bakicelebi
bakicelebi reviewed Jul 11, 2024
View reviewed changes
lib/ui/pages/workspace_page/components/drawing_surface/drawing_canvas.dart Outdated
@@ -123,6 +120,7 @@ class _DrawingCanvasState extends ConsumerState<DrawingCanvas> {
_resetCanvasScale(fitToScreen: isFullscreen);
},
);
final selectedTool = ref.watch(toolBoxStateProvider).currentTool;
return Listener(
Copy link
Contributor

Choose a reason for hiding this comment

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

if (selectedTool is TextTool) ref.watch(textToolProvider)

@bakicelebi
Copy link
Contributor

This PR has a bounding box implementation that can be useful for text tool aswell. 👍🏻

@bhav-khurana
Copy link
Contributor Author

Thanks for this, on it

@bhav-khurana
Copy link
Contributor Author

I have added a basic implementation; can you please review it @bakicelebi @juliajulie95
there's still a bug though where the text in the drawing canvas does not get immediately updated while writing some text in the text box above. Can you also have a look at the same?

@bhav-khurana bhav-khurana changed the title Add Text Tool PAINTROID-759: Add Text Tool Aug 20, 2024
bakicelebi
bakicelebi requested changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@bakicelebi bakicelebi 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.

Please implement reasonable unit, widget, integration testcases for the new tool. Also merge the latest develop to get the latest bounding box implementation

lib/core/commands/command_factory/command_factory.dart
@@ -38,4 +40,12 @@ class CommandFactory {
Offset center,
) =>
CircleShapeCommand(paint, radius, center);

AddTextCommand createAddTextCommand(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be just TextCommand and createTextCommand

lib/core/commands/command_factory/command_factory.dart
TextStyle style,
Paint paint,
) =>
AddTextCommand(point, text, style, paint);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be just TextCommand

lib/core/commands/command_implementation/add_text_command.dart
import 'package:paintroid/core/json_serialization/converter/paint_converter.dart';
import 'package:equatable/equatable.dart';

class AddTextCommand extends GraphicCommand with EquatableMixin {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be just TextCommand

lib/core/commands/command_implementation/add_text_command.dart
}

void undo(Canvas canvas) {
// TODO
Copy link
Contributor

@bakicelebi bakicelebi Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Commands dont have undo method or rather should not have one. Please use the override in the specific tool, in this case text tool.

lib/core/commands/command_implementation/add_text_command.dart
List<Object?> get props => [point, text, paint];

@override
bool? get stringify => true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why nullable?

lib/ui/pages/workspace_page/components/drawing_surface/drawing_canvas.dart
@@ -26,6 +24,7 @@ class _DrawingCanvasState extends ConsumerState<DrawingCanvas> {
var _pointersOnScreen = 0;
var _isZooming = false;
Offset _lastPointerUpPosition = Offset.zero;
final textController = TextEditingController();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is misplaced tool specific controller should be in a tool widget. Its not used anyway.

lib/ui/pages/workspace_page/components/drawing_surface/drawing_canvas.dart
error: (_) => Container(),
loading: (_) => Container(),
),
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.

What is this stacking?

lib/ui/pages/workspace_page/components/top_bar/top_app_bar.dart
ref
.read(canvasStateProvider.notifier)
.resetCanvasWithExistingCommands();
ref.read(toolBoxStateProvider.notifier).switchTool(ToolData.BRUSH);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the switch to brush tool?

lib/ui/pages/workspace_page/workspace_page.dart
@@ -66,7 +68,7 @@ class _WorkspaceScreenState extends ConsumerState<WorkspacePage> {
child: Scaffold(
appBar: isFullscreen ? null : const TopAppBar(title: 'Pocket Paint'),
backgroundColor: Colors.grey.shade400,
resizeToAvoidBottomInset: true,
resizeToAvoidBottomInset: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change necessary?

lib/ui/pages/workspace_page/workspace_page.dart
@@ -28,6 +28,8 @@ class _WorkspaceScreenState extends ConsumerState<WorkspacePage> {
);
}

final textController = TextEditingController();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used.

bakicelebi
bakicelebi reviewed Aug 23, 2024
View reviewed changes
lib/core/tools/text_tool/bounding_box.dart
}
}

void rotate(Offset point) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure rotation should be supported by text tool. If this is a technical limitation of your current implementation it should be disabled in some other way, but definetly implemented before merge.

bakicelebi
bakicelebi reviewed Aug 23, 2024
View reviewed changes
lib/core/tools/text_tool/text_tool.dart

@override
void onDown(Offset point, Paint paint) {
paint = paint;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of this?

@juliajulie95
Copy link
Contributor

@bhav-khurana Hi, are you still working on this or should someone take over?

@bhav-khurana
Copy link
Contributor Author

Hey @juliajulie95
yes it would be great if someone else could do it for now; it will take a while for me to get back to this, sorry for the inconvenience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bakicelebi bakicelebi bakicelebi requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bhav-khurana @bakicelebi @juliajulie95