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

Feat: support for dynamic placeholders #2238

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

CatHood0
Copy link
Collaborator

@CatHood0 CatHood0 commented Sep 18, 2024

Description

Added placeholderComponentsConfiguration parameter to allow us to build placeholders dynamically depending on what attribute we want and cursorParagrahPlaceholderConfiguration that allows to the devs add a placeholder at the right (if we are using a LTR language) of the cursor (only when the paragraph and it does not contains any block attribute or selection)

For example, if we only want to display placeholders for Headers and Lists, then we would set it up like this:

configurations: QuillEditorConfigurations(
  // We can use this instance to avoid use this functionality:
  // CursorParagrahPlaceholderConfiguration.noPlaceholder()
  // Or, if we want to add our custom settings, we can configure manually
  // CursorParagrahPlaceholderConfiguration(paragraphPlaceholderText: "our_text'', style: TextStyle(), show: true);
  // At this example case, we use this instance (just for showcases)
  cursorParagrahPlaceholderConfiguration: CursorParagrahPlaceholderConfiguration.withPlaceholder(), 
  placeholderComponentsConfiguration:
    PlaceholderComponentsConfiguration(
       builders: {
         Attribute.header.key: (Attribute attr, style) {
           final values = [30, 27, 22];
           final level = attr.value as int?;
           if (level == null) return null;
           final fontSize = values[ (level - 1 < 0 || level - 1 > 3 ? 0 : level - 1)];
           return PlaceholderArguments(
             placeholderText: 'Header $level',
             style: TextStyle(fontSize: fontSize.toDouble())
                 .merge(style.copyWith(color: Colors.grey)),
             );
        },
        Attribute.list.key: (Attribute attr, style) {
          return PlaceholderArguments(
            placeholderText: 'List item',
            style: style.copyWith(color: Colors.grey),
          );
        },
      },
   ),
),

Preview

Web

test_web_placeholders.mp4

Desktop

desktop_placeholders.mp4

Android

screen-20240920-163600.mp4

Related Issues

N/A

  • 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 must also make a cursor placeholder that appears at the same offset of the caret on any empty line (if the line has not block or embed attributes)
  • We must make possible add custom block attributes keys (we need to add some validations because we don't want to add any non block attribute)
  • Make this feature completely optional.
  • We need to add support for web
  • Fix issue with the positioning of the placeholders in mobile devices
  • Provide a way to calculate correctly the offset of the placeholder (only for cursor)
  • Test it on all platforms (this is an issue by itself because i don't have IOS or MacOs devices)
  • Test if the feature could cause any issue if we have a line with several block attributes applied
  • Test the system and attribute directionality (RTL, and LTR)
  • Test cursor placeholder
  • Add documentation about this feature.

@CatHood0 CatHood0 added the enhancement New feature or request label Sep 18, 2024
@CatHood0 CatHood0 self-assigned this Sep 18, 2024
@CatHood0 CatHood0 marked this pull request as draft September 18, 2024 01:00
@EchoEllet
Copy link
Collaborator

EchoEllet commented Sep 18, 2024

Seems to be an interesting feature.

I have noticed we're adding new features and then introduce breaking changes after short time to fix a related issue so we probably want to mark the new features as experimental for a while then make them stable similar to how it's done on other project.

For example, the SpellCheckerService needs to be completely rewritten. It should abstract the source of checking the spell of words and not a service for managing a client-side spell checker. The user may want to use server-side solution or a services that provide web API so they can send GET request and get the response back, or they might want to use method channel to access the system spell checker service, the other functions such as dispose and the others shouldn't be part of the interface instead should be only managed by the client side spell checker service provided by the library itself, or maybe we can provide helper class for that, we should only have one method that is a future similarly to Flutter, and that's a breaking change, there are ways make it backward compatible but it's not as clean as removing them and can be confusing.

We can use the experimental annotation from themeta package. It doesn't work similarly to how it works on other languages so the user can compile without opting in.

This PR doesn't seem to have similar issues, but just in case so, we don't have to deprecate them and support them for a while like the enableMarkdownStyleConversion.

This is an optional suggestion.

Copy link
Collaborator

@AtlasAutocode AtlasAutocode left a comment

Choose a reason for hiding this comment

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

I have noticed we're adding new features and then introduce breaking changes after short time to fix a related issue

This morning, I received 10 emails with incremental changes. Most of the changes seem to be addressing minor changes to code that could have been implemented before submitting. I work in a branch of my personal fork, test, implement minor changes and only submit when all is done, code is complete and stable.

I agree with @EchoEllet that new features should be complete before submitting. Bugs and errors in edge cases are inevitable but code needs to be tested before submitting. Code used by our userbase must be stable and complete.

I am sorry to be critical because you are working so hard at improving the project. But there are an increasing number of critical issues that are not being addressed. As a couple of examples:

For example, the SpellCheckerService needs to be completely rewritten.

Spell checking might be very important for some users, but the penalty is too high. The implementation is completely useless for me as I need a highly technical vocabulary that is never going to be implemented.

Embedding Table

Still not implemented? Please fix or remove. Or at least mark as experimental so users know to avoid. This is a perfect example of working in a private branch until the feature is complete and ready for general use.

I am having a bad day. I apologize if my comments are too harsh. I am getting frustrated.

placeholder appear on any empty line

NO!!!

P.S. I have not reviewed the code changes.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Sep 18, 2024

Thank @AtlasAutocode for the feedback, I do agree that we should test our changes in the dev branch and push them as pre-release since the testing is very manual. Marking them as experimental is the least we can do.

We should be more cautious when introducing new changes since medium-big projects depend on this project. Users expect the changes to be stable in the master or the stable release.

This is a mistake I made in 2023 when working on this repository since I didn't even have the time to write a good commit message or explain my changes (See #1566 as a perfect example). Now I'm introducing fewer features and working less often than before but all of my changes are much more stable now. At the end of 2023, I only had 1 hour in a month.

I'm able to reproduce bugs that are not in the issue that we can't keep track of and have been since 2023 (example).

For now, I suggest making most of our new changes in completely separate files to make it easier to keep track of things, we don't want to make the QuillRawEditorState and QuillController and related files 40% bigger.

Use part and part of, extensions, private functions, classes. We need more code reviews like before, see 177ddf6428295d725e87 as a recent example.

Once I'm done with #2230. I will separate the magnifier into its own file (#2026, #2026).

I do appreciate your time and efforts.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Sep 18, 2024

We can use the experimental annotation from themeta package. It doesn't work similarly to how it works on other languages so the user can compile without opting in.

Thanks for the suggestion. I'll take it into consideration.

This PR doesn't seem to have similar issues, but just in case so, we don't have to deprecate them and support them for a while like the enableMarkdownStyleConversion.

You are correct, there are no issues related to placeholders. However, I thought we could add this functionality to improve how we view elements that have attributes in blocks, such as lists, titles, and even blockquotes that when empty, do not display any text, which is sometimes confusing (at least in my case it has been).

I take BlockNote editor as an example of how this feature should be maded:

Captura de pantalla_2024-09-18_11-53-37

For example, the SpellCheckerService needs to be completely rewritten. It should abstract the source of checking the spell of words and not a service for managing a client-side spell checker. The user may want to use server-side solution or a services that provide web API so they can send GET request and get the response back, or they might want to use method channel to access the system spell checker service, the other functions such as dispose and the others shouldn't be part of the interface instead should be only managed by the client side spell checker service provided by the library itself, or maybe we can provide helper class for that, we should only have one method that is a future similarly to Flutter, and that's a breaking change, there are ways make it backward compatible but it's not as clean as removing them and can be confusing.

I will take this into account to improve the problems we are having with the current spell checker. Anyway, thanks for the suggestion.

For example, the SpellCheckerService needs to be completely rewritten.

Spell checking might be very important for some users, but the penalty is too high. The implementation is completely useless for me as I need a highly technical vocabulary that is never going to be implemented.

You have a point. But, some devs could need this feature. I'll add it to my TODO list. Thanks for your feedback.

Embedding Table

Still not implemented? Please fix or remove. Or at least mark as experimental so users know to avoid. This is a perfect example of working in a private branch until the feature is complete and ready for general use.

I think the same. Table should not be available for users. I'll make a PR to disable it by now because is quite experimental and it does not work as we expect. I'll working on it to improve our implementation.

placeholder appear on any empty line

NO!!!

It seems I expressed myself poorly. I blame my PC for not being able to properly record a sample video for this feature (no idea).

At this point, what I mean is that if we create our own placeholders using placeholderComponentsConfiguration whenever these lines are empty, their intended placeholder will appear. However, this is ONLY limited to the exclusive attributes i.e. header, list, code-block, and blockquote. Any other paragraph-like or inline attributes (alignment, line-height, direction and similar attributes are also ignored to avoid putting unwanted placeholders)

Now, we have the case of the placeholder that appears at the cursor level. This is limited to only appearing IF the line is empty and it does not have any block or embedded attributes. That is, it is a full-fledged paragraph.

Needless to say, by default, none of these features are enabled. It is up to the user to configure them. If they are not desired, one can simply pass null, or not call them in the constructor.

Note

If you have time, please, check BlockNote. This feature works in a very similar way to how this editor works with its placeholders.

I'm having a bad day. I apologize if my comments are too harsh. I'm getting frustrated.

Don't worry, man. There are always those kinds of days. Thanks for your feedback, I'll take it into account for any future PR I'm going to do.

I agree with @EchoEllet that new features should be complete before submitting. Bugs and errors in edge cases are inevitable but code needs to be tested before submitting. Code used by our userbase must be stable and complete.

I completely agree. That's why many of the PRs I've done take their time to be tested and to ensure that there are no problems. Anyway, thanks for mentioning it.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Sep 18, 2024

I will take this into account to improve the problems we are having with the current spell checker. Anyway, thanks for the suggestion.

I do suggest that we discuss it before attempting to introduce any changes, I do plan to at least try to use the DefaultSpellCheckerService from Flutter. if it doesn't work will implement it using quill_native_bridge.

You have a point. But, some devs could need this feature. I'll add it to my TODO list. Thanks for your feedback.

Another mistake I made in 2023 is introducing any feature that is needed by all developers. If this is not something commonly known and used then it shouldn't be a feature since it can cause issues for the other 95% developers. Instead, we should make it possible to implement to the 5% developers in a clean way that doesn't cause breaking changes in the future. Having an 80 MB file on a client app (especially on the web) is not ideal which is why is one reason why I moved it into the example. Even if this is fixed after very short time, it can give a negative impression to users, which is the main reason I suggest discussing this before proceeding further, and another reason why I invited you to the development channel on Slack recently. Different developers have experience in different areas.

I completely agree. That's why many of the PRs I've done take their time to be tested and to ensure that there are no problems. Anyway, thanks for mentioning it.

Sure but not all issues can be encountered by testing the feature in working time. Today I was able to detect an issue on Android that only occurs when copying an image from another app and when pasting it, then closing/restarting the app and attempting to paste it once again.

This will cause a security exception to be thrown each time opening the app, and the only workaround is to ask the user to clear their clipboard or copy something different and we can't since the app won't open.

the app will keep crashing infinitely until the user deletes the app or the system asks them to. This was an issue in quill_native_bridge and super_clipbaord (almost all clipboard plugins that support Android + some native Android apps).

I provided a workaround (return null instead of crashing the whole app - disable the feature) I wouldn't be able to detect this (in addition to many other issues) if I didn't 4 days for #2230. I could have marked it as ready for merge/review on the first day and then submitted PRs fixing bugs and incomplete features. Every user/developer will be a test site.

See super_native_extensions #435 for more details about this issue.

Note

Used www.github.com instead of github.com in the link above to avoid linking/referencing unrelated issues on their repo

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Sep 19, 2024

The issue with the indent was moved to #2253

}

bool _isNodeInline(Line node) {
for (final attr in node.style.attributes.values) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I notice of this code could not be here. I will make this part of an extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants