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.

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

Chore: Replace flutter-quill with visual-editor #3

Closed
2 tasks done
nelsonic opened this issue Sep 6, 2023 · 16 comments · Fixed by #7
Closed
2 tasks done

Chore: Replace flutter-quill with visual-editor #3

nelsonic opened this issue Sep 6, 2023 · 16 comments · Fixed by #7
Assignees
Labels
chore a tedious but necessary task often paying technical debt documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality flutter Flutter related issues help wanted If you can help make progress with this issue, please comment! priority-1 Highest priority issue. This is costing us money every minute that passes. T1d Time Estimate 1 Day tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written technical A technical issue that requires understanding of the code, infrastructure or dependencies

Comments

@nelsonic
Copy link
Member

nelsonic commented Sep 6, 2023

As noted by @LuchoTurtle in singerdmx/flutter-quill#1389 a brick wall was hit in terms of testing with flutter-quill. 🧱 😢

Therefore we need to:

  • Switch this tutorial to using visual-editor as fast as possible, ideally in less than a day.
  • Please leave regular comment updates on this issue or your Pull Request so that it's clear when you are having difficulty.

This should be straightforward as visual-editor is a fork of flutter-quill so there should be significant overlap/similarity.
If there is a lot of divergence, and it's not a "find and replace" job, please leave comments! 💬 🙏

@nelsonic nelsonic added documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality help wanted If you can help make progress with this issue, please comment! flutter Flutter related issues chore a tedious but necessary task often paying technical debt technical A technical issue that requires understanding of the code, infrastructure or dependencies T1d Time Estimate 1 Day tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written priority-1 Highest priority issue. This is costing us money every minute that passes. labels Sep 6, 2023
@LuchoTurtle
Copy link
Member

LuchoTurtle commented Sep 6, 2023

I'm trying to install visual-editor but I'm getting an error resolving dependencies:

Resolving dependencies... (1.8s)
Because no versions of mockito match >5.4.2 <6.0.0 and mockito 5.4.2 depends on analyzer ^5.11.0, mockito ^5.4.2 requires analyzer ^5.11.0.
And because every version of visual_editor from git depends on i18n_extension ^4.2.1 which depends on analyzer ^3.0.0, mockito ^5.4.2 is incompatible with visual_editor from git.
So, because app depends on both visual_editor from git and mockito ^5.4.2, version solving failed.

I'm using mockito just as a dev dependency. But nonetheless, the package i18n_extension that visual-editor uses should be upgraded so version solving succeeds 💭

Created issue on visual-space/visual-editor#234

@nelsonic
Copy link
Member Author

nelsonic commented Sep 6, 2023

Definitely open that issue on the project repo and propose the update. 👌

@LuchoTurtle
Copy link
Member

LuchoTurtle commented Sep 11, 2023

Just going to give an update on this issue. The reason it's not been migrated yet is because the visual_editor was using outdated dependencies get resulting in dependency versioning errors when trying to install.

The example of the package was only working on an older version of Flutter, which is not desirable because it would break versioning on our app.

This is why the following issues and updates have been opened, linked here and occured in the following:

I'm practically done with the PR that updates the dependencies that should hopefully fix my blocker when installing and migrating flutter_quill to visual_editor.

I haven't opened the PR yet because, as per visual_editor's guidelines, it's one commit per PR. I need to do some final tests to make sure their example app works fine as before (it does on both iOS and Android, just need to finish checking on the web) and squash all the commits into one so I can open a PR. I'm currently waiting on a medical appointment, so I'll do that once that's sorted.

@LuchoTurtle
Copy link
Member

Created visual-space/visual-editor#237 to address these concerns.

@LuchoTurtle
Copy link
Member

I've added changes to the PR visual-space/visual-editor#237 related to visual-space/visual-editor#238. It's needed because it's using a library gallery_saver that is not maintained and depends on an older version of http.

This will make it impossible to install visual-editor within our app. Hopefully the PR gets merged so the migration can be done instantaneously afterwards 👌

@nelsonic
Copy link
Member Author

@LuchoTurtle please use your fork for working on the replacement.
ref: visual-space/visual-editor#237 (comment)
Thanks.

@LuchoTurtle
Copy link
Member

LuchoTurtle commented Sep 19, 2023

Migrating is taking much more time than usual. The migration guide, although useful, is a bit incomplete. And the fields between flutter-quill and visual-editor are not 1:1 (quite the opposite, most of the times).

Some features are missing (setting subscript and superscript, for example). As I'm diving into their code (much more organized and documented, I ought to admit), I'm adding information to their migration-guide.md. Haven't opened a PR to their repo yet because I'm still having errors on tests, so I have to get through those first.

@LuchoTurtle LuchoTurtle moved this to 🏗 In progress in dwyl app kanban Sep 19, 2023
@LuchoTurtle
Copy link
Member

LuchoTurtle commented Sep 19, 2023

I keep getting this darn error when I tap on mobile devices after initializing the app:

════════ Exception caught by gesture ═══════════════════════════════════════════
The following _TypeError was thrown while handling a gesture:
type 'Null' is not a subtype of type 'BlockM' in type cast

The error is here.

https://github.com/visual-space/visual-editor/blob/3747d0e2f43cd88b253df766f201ecfc8045fee6/lib/document/controllers/document.controller.dart#L412-L423

I don't know where this gets called on tapUp and errors out. The document is empty, so there are no nodes and, since it's null, casting fails. But I don't know what I'm meant to return if the node is null :/

I'll see if I can solve the issue locally before opening an issue 👌

@LuchoTurtle
Copy link
Member

Created the issue in visual-space/visual-editor#239.

@LuchoTurtle
Copy link
Member

For some reason, after revisiting this, XCode seems to have uninstalled iOS 17 (?)

Getting this error while running the app.

Failed to build iOS app
Could not build the precompiled application for the device.
Uncategorized (Xcode): Unable to find a destination matching the provided destination specifier:
		{ id:00008030-0018253C3E85802E }

	Ineligible destinations for the "Runner" scheme:
		{ platform:iOS, id:dvtdevice-DVTiPhonePlaceholder-iphoneos:placeholder, name:Any iOS Device, error:iOS 17.0 is not installed. To use with Xcode, first download and install the platform }

I guess I'll install it (again)?

image

@nelsonic
Copy link
Member Author

yeah, this kind of XCode crap happens occasionally when there are major updates (like annually). 🙃
Were you able to re-install everything and get it working again? 💭

@LuchoTurtle
Copy link
Member

image

It's going to take a while. I'll use my Android for now.

@nelsonic
Copy link
Member Author

That's nuts! Are we on dial-up? 🐌
Glad you've got something to fallback on. 👌
We might want to just restart all the internet at the mains switch? 💭

@LuchoTurtle
Copy link
Member

I thought I got it fixed but turns out that me checking for the null in https://github.com/visual-space/visual-editor/blob/3747d0e2f43cd88b253df766f201ecfc8045fee6/lib/document/controllers/document.controller.dart#L412-L423 is making copy and pasting completely broken.

In addition to this, running the original example from their repo, selecting text and copy/pasting doesn't properly work. Having perused their code, this also makes sense, since apparently these methods are not being called.

https://github.com/visual-space/visual-editor/blob/3747d0e2f43cd88b253df766f201ecfc8045fee6/lib/selection/controllers/selection-handles.controller.dart#L117-L141

Here's what happens:

23-09-20-11-38-48.mp4

When I try to make changes to fix the issue opened in visual-space/visual-editor#239, the error does not pop up, but the behaviour is the same. Here's how I'm trying to fix it:

  NodePositionM queryChild(int offset) {
    final nodePos = _contUtils.queryChild(rootNode, offset, true);

	// Changing here. I'm checking if it's null. If it is, I'll return the `rootNode.defaultChild`, because it's the only way of returning a `LineM` object.
    if (nodePos.node == null) {
      return NodePositionM(rootNode.defaultChild, 0);
    }

    if (nodePos.node is LineM) {
      return nodePos;
    }

    final block = nodePos.node as BlockM;

    return _contUtils.queryChild(block, nodePos.offset, true);
  }

I can create the PR for the fix (alongside another Null error that occurs when the toolbar shows up ) to see if they comment.

But as it stands, I don't think visual-editor is a viable option for us. However I urge you to merge this PR, even if it's connected to my fork. We can create a new one that will connect to the real repo once the changes are merged and these problems are fixed.

I'm going to alter this PR so the original files are not changed. I'll just put the migration files in their dedicated folder so people who see this repo are able to see both versions.

@LuchoTurtle
Copy link
Member

Created both of these PRs:

@LuchoTurtle
Copy link
Member

https://github.com/AppFlowy-IO/appflowy-editor might be another option to consider, as it provides Delta/Quill support, unlike https://github.com/superlistapp/super_editor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore a tedious but necessary task often paying technical debt documentation Improvements or additions to documentation enhancement New feature or enhancement of existing functionality flutter Flutter related issues help wanted If you can help make progress with this issue, please comment! priority-1 Highest priority issue. This is costing us money every minute that passes. T1d Time Estimate 1 Day tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants