flutter-quill icon indicating copy to clipboard operation
flutter-quill copied to clipboard

Chore: move editor text elements into separate files when possible

Open CatHood0 opened this issue 1 year ago โ€ข 2 comments

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.
  • [x] ๐Ÿงน 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.
  • [x] ๐Ÿ—‘๏ธ 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.

CatHood0 avatar Sep 23 '24 19:09 CatHood0

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

EchoEllet avatar Sep 23 '24 19:09 EchoEllet

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 avatar Sep 23 '24 21:09 EchoEllet

I apologize for the inactivity I have had. In one or two days I will start again, resume activity and continue with the PRs that I have active.

Anyway I have to take some time to remember what I was doing in this PR.

CatHood0 avatar Oct 16 '24 22:10 CatHood0

@CatHood0 Do you want me to handle those conflicts? If yes, then I need to push changes to your fork or create a new PR since I'm unable to push directly or merge with master after the updates (insufficient permission).

EchoEllet avatar Nov 14 '24 11:11 EchoEllet

@EchoEllet there's no problem for you to make a new PR if necessary. Thank you very much for taking the time.

CatHood0 avatar Nov 14 '24 17:11 CatHood0

Can you push your branch to this repo instead so we can both make changes?

Since I only plan to make minor changes, including fixing conflicts.

EchoEllet avatar Nov 14 '24 17:11 EchoEllet

Can you push your branch to this repo instead so we can both make changes?

It's ready now.

CatHood0 avatar Nov 14 '24 17:11 CatHood0

This will require more work.

I'm still trying to figure out why some classes are used and exist and why certain things exist (this took me too long). I definitely need to document everything, it's really tiring to see so many RenderObjectElement or RenderBox or RenderContentProxyBox without clear and concise documentation.

It was even confusing to know why RenderParagraphProxy existed, and how RichTextProxy managed to use it so that RenderEditableTextLine and _TextLineElement were used as the "body" using setBody in RenderEditableTextLine to have the methods that RenderContentProxyBox contains (i mention it in the past tense because i already understood how it is done, but it took me too long to understand the connection between all these classes)..

CatHood0 avatar Jan 28 '25 00:01 CatHood0

I definitely need to document everything, it's really tiring to see so many RenderObjectElement or RenderBox or RenderContentProxyBox without clear and concise documentation.

This is a bit out of scope for this PR, or maybe too much? I think it's enough to only separate classes into files.

EchoEllet avatar Jan 28 '25 00:01 EchoEllet