Chore: move editor text elements into separate files when possible #2274

CatHood0 · 2024-09-23T19:03:54Z

Description

The decision was made to separate the text element widgets into their own separate files in order to not only separate the logic, but to make it more readable and easy to debug.

This decision was made since there are many files with this pattern, of having a lot of internal logic that is not completely related in the same place, being that we could separate them into different files.

If this PR is merged, we will likely continue to do this type of refactoring since it improves readability and makes it easier to implement solutions since we do not have to read a 1600 line file.

arabianRomanNumbers and romanNumbers from text_block file because they were no longer used internally and were just taking up unnecessary space.

Note

I did not separate the members within text_selection because @EchoEllet is currently making changes within this file, so it would be better to wait for them to complete and then split them.

✨ New feature: Adds new functionality without breaking existing features.
🛠️ Bug fix: Resolves an issue without altering current behavior.
🧹 Code refactor: Code restructuring that does not affect behavior.
❌ Breaking change: Alters existing functionality and requires updates.
🧪 Tests: Adds new tests or modifies existing tests.
📝 Documentation: Updates or additions to documentation.
🗑️ Chore: Routine tasks, or maintenance.
✅ Build configuration change: Changes to build or deploy processes.

TODO:

we need to test if these changes don't break nothing.
we need to add some documentation easy to understand for the new files.

EchoEllet · 2024-09-23T19:09:53Z

Will review it soon. It's usually more effective to review those changes in the IDE or command line instead of GitHub Web.

EchoEllet

Haven't reviewed the PR yet.

lib/src/editor/widgets/text/block_line/text_block.dart

AtlasAutocode

Looks OK to me

EchoEllet · 2024-09-23T21:17:08Z

Still need a bit more time before working on #2251 or fully reviewing this change. We need some tests to ensure that we're not exposing any file that have @internal in it to ensure that user can't accidentally use an internal API. I do get reports about breaking changes when it's not clear that this is an internal API or accidentally made out to the public by a PR.

Added a breaking changes section to explain this in #2275 though still not quite enough. Should require minimal effort to fix. The issue that we're not using @internal when we can and that's something to address.

EchoEllet · 2024-09-24T16:08:17Z

lib/src/editor/widgets/text/block_line/render_editable_text_block.dart

+
+  @override
+  TextPosition? getPositionAbove(TextPosition position) {
+    assert(position.offset < container.length);


We need to add a message for all asserts, even if we're not sure about the check and why it's done, a message or something like:

Position offset ($position.offset) must be less than the container length (${container.length}).

EchoEllet · 2024-09-24T16:10:11Z

lib/src/editor/widgets/text/block_line/render_editable_text_block.dart

+
+  @override
+  TextPosition? getPositionBelow(TextPosition position) {
+    assert(position.offset < container.length);


This seems to be the same assert check as the one in getPositionAbove. Also need a message.

EchoEllet · 2024-09-24T16:15:29Z

lib/src/editor/widgets/text/block_line/text_block.dart

+import 'package:flutter/material.dart'
+    show
+        Border,
+        BorderSide,
+        BoxDecoration,
+        BuildContext,
+        Color,
+        Colors,
+        Directionality,
+        EdgeInsets,
+        FontWeight,
+        MediaQuery,
+        StatelessWidget,
+        TextDirection,
+        TextRange,
+        TextSelection,
+        ValueChanged,
+        Widget,
+        debugCheckHasMediaQuery,
+        immutable;


Let's remove the show here and only use it when we it for other reasons such as this line since I accidentally used exitProcess instead of the exitCode of the xclip process.

EchoEllet · 2024-09-24T16:15:55Z

lib/src/editor/widgets/text/block_line/editable_text_block.dart

+import '../../../../document/nodes/block.dart';
+import 'render_editable_text_block.dart';
+
+//TODO: we need to document what does this and why we use it


@AtlasAutocode @singerdmx Any idea?

It looks like this was being used to separate the widget logic from the build and rendering logic.
For a StatefulWidget, we would use WidgetState to achieve the same purpose.
I guess I would leave it as a private class only callable from its parent EditableTextBlock. If we extract it as a separate public class, it could be mis-called from who knows where. (And people will ask 'why do we use this')

I will add some documentation about what makes this class. We have some docs about this, but i have no much time. I will search and add it correctly as soon as possible.

I wrote that TODO to remember that i need to add it.

Take as much time as you need.

EchoEllet · 2024-09-24T16:16:18Z

lib/src/editor/widgets/text/line/editable_text_line.dart

+import 'render_editable_text_line.dart';
+import 'render_text_line_element.dart';
+
+//TODO: we need to document this render object


@singerdmx Same question

EchoEllet · 2024-09-24T16:19:00Z

lib/src/editor/widgets/text/line/render_text_line_element.dart

@@ -0,0 +1,114 @@
+import 'package:flutter/material.dart'
+    show


Same as before, once we start importing more classes and properties, it will become bigger and a bit harder to review. Also, requires changes when we remove something.

EchoEllet · 2024-09-24T16:19:24Z

lib/src/editor/widgets/text/line/render_text_line_element.dart

+    assert(_slotToChildren.containsValue(child));
+    assert(child.slot is TextLineSlot);
+    assert(_slotToChildren.containsKey(child.slot));
+    _slotToChildren.remove(child.slot);


Those do need messages as well.

EchoEllet · 2024-09-24T16:19:42Z

lib/src/editor/widgets/text/line/render_text_line_element.dart

+  @override
+  void update(EditableTextLine newWidget) {
+    super.update(newWidget);
+    assert(widget == newWidget);


Same to all the other asserts, we can make it in a separate PR if you prefer.

EchoEllet · 2024-09-24T16:20:32Z

lib/src/editor/widgets/text/line/render_text_line_element.dart

+
+  @override
+  void insertRenderObjectChild(RenderBox child, TextLineSlot? slot) {
+    // assert(child is RenderBox);


Why this was commented? We should check the history and see if it was commented by a PR that introduced a feature or some other changes. If there is a reason, then it should be removed.

I have found that some issues are reported as causing an assert failure.

A contributor then comments out the assert without searching for why the assert failed.
In my experience, asserts are indicators of a logic error that occurred prior to the assertion.

It is imperative to find the originating cause rather than just ignoring the problem which inevitably results in a future serious failure that is hard to fix. But this is a maintainability issue that most people are not sensitive to since they have not had to maintain a project for years.

I found a similar issue in the magnifier PR, but I didn't have a chance to revert it, though I will soon.

To make it easier for future developers and to keep track of code quality best practices, we need general guidelines and common code quality standards that must be followed consistently throughout the project. A file somewhere in the doc directory or in CONTRIBUTING.md.

EchoEllet · 2024-09-24T16:21:33Z

lib/src/editor/widgets/text/line/render_text_line_element.dart

+  }
+
+  @override
+  void moveRenderObjectChild(


We need to know why this is not implemented and is not needed, same as the other methods that need to be implemented but always throw something like UnimplementedError or similar.

EchoEllet

Thank you for the cleanup.

Chore: move editor text elements into separate files when possible

7b8c5dd

CatHood0 added documentation Improvements or additions to documentation in_progress This issue or feature is currently being worked on by someone. labels Sep 23, 2024

CatHood0 self-assigned this Sep 23, 2024

CatHood0 requested a review from EchoEllet September 23, 2024 19:04

CatHood0 marked this pull request as draft September 23, 2024 19:04

CatHood0 requested review from singerdmx and AtlasAutocode September 23, 2024 19:04

Fix: leaf.CustomBlockEmbed is missing

1a5bc13

singerdmx approved these changes Sep 23, 2024

View reviewed changes

EchoEllet reviewed Sep 23, 2024

View reviewed changes

lib/src/editor/widgets/text/block_line/text_block.dart Show resolved Hide resolved

AtlasAutocode approved these changes Sep 23, 2024

View reviewed changes

EchoEllet reviewed Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: move editor text elements into separate files when possible #2274

Chore: move editor text elements into separate files when possible #2274

CatHood0 commented Sep 23, 2024 •

edited

Loading

EchoEllet commented Sep 23, 2024

EchoEllet left a comment

AtlasAutocode left a comment

EchoEllet commented Sep 23, 2024 •

edited

Loading

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

AtlasAutocode Sep 24, 2024

CatHood0 Sep 24, 2024 •

edited

Loading

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024 •

edited

Loading

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

AtlasAutocode Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet Sep 24, 2024

EchoEllet left a comment

Chore: move editor text elements into separate files when possible #2274

Are you sure you want to change the base?

Chore: move editor text elements into separate files when possible #2274

Conversation

CatHood0 commented Sep 23, 2024 • edited Loading

Description

TODO:

EchoEllet commented Sep 23, 2024

EchoEllet left a comment

Choose a reason for hiding this comment

AtlasAutocode left a comment

Choose a reason for hiding this comment

EchoEllet commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CatHood0 Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EchoEllet Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EchoEllet left a comment

Choose a reason for hiding this comment

CatHood0 commented Sep 23, 2024 •

edited

Loading

EchoEllet commented Sep 23, 2024 •

edited

Loading

CatHood0 Sep 24, 2024 •

edited

Loading

EchoEllet Sep 24, 2024 •

edited

Loading