Change bold and italic to toggle on/off

[stemuk]

I would suggest to change the bold and italic text style buttons to toggle their respective styles on or off for all following entries.

The current implementation only allows for changing currently marked text to bold/italic, it however makes things very complicated if you mark your last typed text as bold/italic, since this means that all following text is automatically marked as bold or italic without the simple option to turn it off again.

IMHO the option to mark selected text as bold or italic should be kept, if no text is currently selected however the respective buttons should toggle the text styles on/off as described above.

[pulyaevskiy]

If I understand this correctly this issue is about toggling inline style(s) with collapsed selection, right?
E.g. when selected range is zero and we see caret at specific position.

So toggling inline style at caret position will affect style of all characters inserted later by the user.

Why I prefer to phrase it this way is because this operation is not a document change, there is no character style to change when toggling with collapsed selection.

Also, current behavior IS a toggle on/off, it just works for non-empty selection. I'll update the title to reflect this.

This would probably have to be implemented as an extension to existing heuristic rules since we need to keep this information somewhere outside of the document model.

[stemuk]

My mistake, I should have labeled this as a possible bug and cross-tested this issue on iOS.
At the moment if you use the bold or italic buttons and no text is marked on Android, nothing happens and the text that gets entered after the button press does not change its style. So if I understand the behaviour you layed out above correctly, the current behaviour on Android is in fact a bug and not the expected behaviour. The source of this bug might again be the GBoard or maybe it is just an Android specific flutter issue.

If you have troubles reproducing the issue I could recod some footage of the bug since no errors are thrown.

[stemuk]

Did you have success recreating the issue on Android so far? If not I could provide footage of the issue or a Stacktrace if that helps fixing it.

[pulyaevskiy]

Haven't had a chance to look into this in the past two weeks.

It would definitely help if you can provide more details here so feel free to add as much as you can!

[stemuk]

Sorry for the late response, I finally found time to record some footage of the toggle on/off issue (at least the current behaviour on Android (iOS?) seems to differ from your described behaviour).

Video: https://streamable.com/s/nslk6/luxudp

The video pretty much shows that on the latest version (0.1.3) the bold and italic text style options do not toggle on or off, but instead work only if text is marked. Having the buttons turn on their respective style options for all following text until turned off would be more intuitive for most users IMHO.

On a side note, I also recorded some footage of the already known divider and list bugs, since they unfortunately do not throw an error and instead occur 'silently'.

Divider issue: https://streamable.com/dhuwx

List / GBoard issue:
https://streamable.com/va3ox

Regarding the second issue, it seems as though only recommended words are affected since they seem to be input differently than organically typed words (notice how at the end of the clip the fully typed word 'red' caused no problems, just typing 're' and selecting the recommended 'red' however did).

I hope this helps, if the issues had thrown any errors I would have included them for easier bug fixing, but unfortunately the issues are only visible within the app, not the console.

[stemuk]

@pulyaevskiy Did the recorded footage help replicating the issues described above? To me it seems like the issues mainly appear on Android since the iOS keyboard does not seem to have the same behavior when it comes to auto-correcting text.
Please let me know if I can help in any way.

[pulyaevskiy]

@stemuk Hey, sorry for being slow here.

I haven't been able to spend much time on these, but videos are incredibly helpful, really appreciate your time!

Divider issue: this should be fixed in the latest master. I haven't published an updated version yet though. See 19da3da for details.

Bold/italic issue: I think I agree with your points. I have a rough idea on how to implement this, just need to get some actual time.

Android autocomplete issue: I still need to do more debugging on this one, not much I can say at the moment. I'll do another run on this one today/tomorrow though.

[pulyaevskiy]

I just pushed an update to master which should solve the "autocorrect" issue on Android.

You should be able to test it by configuring dependency overrides in your pubspec:

dependency_overrides:
  zefyr:
    git:
      url: https://github.com/memspace/zefyr.git
      path: packages/zefyr
  notus:
    git:
      url: https://github.com/memspace/zefyr.git
      path: packages/notus

Let me know if this works for you. I'll probably publish a new version with these changes if all looks good.

[stemuk]

@pulyaevskiy Thanks a lot for your work, the autocorrect issue seems to be fully fixed! 👍
As for the divider issue I just implemented the dependency overwrites and so far the divider issue still seems to be prevalent in some situations, and somehow the changes have brought some new issues surrounding the divider functionality.

To demonstrate this I can for now only provide screenshots, I will try to record some footage as soon as I can:

1. This error message appeared after trying to select the divider: https://i.imgur.com/zZqnp5N.png
   The same error seems to be occurring for clicking on empty list items and on areas outside the current document, so it seems the error is not exclusive to dividers.
   It seems like adding a simple check in the case a document-item has no pasteButtonLabel would do the job.
   Edit: Here is some footage of the issue: https://streamable.com/9vcsz

2. Additionally, in the case of trying to create a divider in the last line of a document the old issue seems to reappear: https://i.imgur.com/3lAC3aQ.png

Edit: I was having issues recreating this issue on an actual device, the first testing was conducted on an emulator.
A smaller (but still important) issue that I additionally found was that it is impossible to delete a divider if the user sets his cursor into the next line on the left (the divider can only be deleted if the cursor is weirdly set on the right side of the divider in the same line, which seems very unintuitive to me).
Footage: https://streamable.com/eb34s

Console output:
I/flutter ( 5421): Another exception was thrown: NoSuchMethodError: The getter 'pasteButtonLabel' was called on null.

3. The text input by a user after creating a divider appears above the divider automatically, even if the user selects the line directly below the divider will cause new text to appear in the line above the divider.

Edit: Just for clarification: My expected behavior would be that if a divider is used on an empty line, the divider would fill up that line, a new line is created and the cursor is set at the start of the new line.
In the case of using a divider in a line that is already filled with text I would expect the divider to appear in new line beneath the text, and my cursor to be put in a new line beneath the divider (so effectively 2 new lines created).

[stemuk]

@pulyaevskiy I updated my previous comment with some footage and added some comments. Please let me know what you think!

[pulyaevskiy]

Thanks again for a lot of useful info!

The error you see with paste button is definitely something that should be fixed.

A smaller (but still important) issue that I additionally found was that it is impossible to delete a divider if the user sets his cursor into the next line on the left (the divider can only be deleted if the cursor is weirdly set on the right side of the divider in the same line, which seems very unintuitive to me).

I actually find it intuitive and consistent with how deleting works with regular text. E.g. say I have following text:

First line
|Second line

Pipe | represents cursor position. If I press backspace the two lines are merged together. But according to your logic it should delete the S on second line.

The same way with embeds. If cursor is before an embed (divider, image) and you press backspace it deletes character before that embed, which is a line-break. In this context it works as expected as I see it.

Note that pressing backspace when embed is right after a non-empty line does nothing (as you see it on your recording). This is because embeds occupy the whole line and therefore cannot exist together with other things on that same line. Basically this is also expected and prevents document getting into a state which cannot be represented (rendered) by Zefyr.

[stemuk]

@pulyaevskiy Thanks again for your fast response! As for the expected behaviour around deleting a divider:

The way I see it, this https://streamable.com/eb34s clip describes unintuitive behaviour in the way that even though my cursor was in the last line of the document, multiple presses of the back button did not delete the divider above my cursor.
(Edit: I just watched the clip again and it might be that a bug caused the cursor to be squeezed into the same line as the divider even though the cursor is beneath it, since putting the cursor manually into the line below the divider makes the cursor-divider distance much larger...)

Since taps are visualized in this video, notice how I tapped the back button multiple times near the end of the clip without being able to delete the divider.

I just recorded an extended clip https://streamable.com/vmdf3 that both shows the expected behaviour which seems to be prevalent if you create a divider in between already created (empty) lines and the unintuitive behaviour I layed out which occurs if you create a divider at the last line (bottom) of your document.

Since this is likely a place where most editing (and divider-creation) will occur I believe that this behaviour should be changed in order to be consistent with the behaviour you mentioned (a simple back-button press should delete the freshly created divider instead of blocking all deletions unless the cursors position is changed).

[pulyaevskiy]

OK, I think I'm starting to see where your confusion comes from.

As I mentioned in my previous comment when there are empty lines before a divider pressing backspace removes those empty lines. However when you added "Just a test" on a line right before a divider it results in a no-op action. This is because divider can not be merged with the text above it.

Now, I think I have a solution for this case. Which is instead of current no-op behavior we can make the divider selected when user taps on backspace (so that it appears with selection background and handles). So next time user taps on backspace the divider is gone.

How does this sound?

Also note that you can move caret by tapping on the left or right side of a divider (works with images as well). If you tap on the right side the caret would move past the divider so you can now delete it by tapping on backspace.

I see a few separate issues that can be addressed here:

1. When divider is inserted position caret after it, not before as it currently does. This would make deletion a bit easier. And looks like would mostly satisfy your scenario.
2. When user attempts to delete a line break before a divider and previous line is not empty make the divider selected instead of defaulting to no-op. This could have edge cases when previous line contains another embed (image) and I feel like in this case it would be more accurate to select embed from the previous line.

[pulyaevskiy]

Great discussion, by the way. Thanks again for tracking down this issues.

[pulyaevskiy]

@stemuk

I just pushed a small update which addresses point (1) - positions caret after an embed on insert.

Re: pasteButtonLabel exception

I couldn't find any references to this property in Zefyr source code and I couldn't reproduce the error on my end.

I'm suspecting that this could be somewhere in your app, could you check and let me know?

[stemuk]

@pulyaevskiy Thanks a lot for the quick fix, I think your solution works really well. As for the pasteButtonLabel:
I recorded all the footage above in the Zefyr editor example to make reproduction easier, which is why I would suspect that the pasteButtonLabel is not in the app itself.

If I recall correctly you mentioned in another issue that you are currently using the build-in flutter labels for Copy, Cut and Paste operations and not custom ones.
In this case I would expect the pasteButtonLabel property to exist in the build-in flutter implementation of the Cut, Copy and Paste labels, which is why a fix would likely include a check if the selected item is text or not.

At the moment it seems like the error is occurring by selecting everything except text, since even selecting an image in a document triggers the issue.

[stemuk]

@pulyaevskiy It seems to me that pasteButtonLabel is a property of flutter localisation classes like the DefaultMaterialLocalizations class https://docs.flutter.io/flutter/material/DefaultMaterialLocalizations/pasteButtonLabel.html . This is probably the reason why you couldn't find the property in the Zefyr editor.

[pulyaevskiy]

@stemuk yeah, thanks! You are right.

Strange though I couldn't reproduce it on emulator. Wondering which locale gets initialized on your end for the app?

[stemuk]

@pulyaevskiy My locale is German, but I just cross checked and the issue can be reproduced in English and French as well, which would suggest that it is not locale exclusive.

[stemuk]

@pulyaevskiy It seems there is a patch coming soon that might address this issue: flutter/flutter#22624

[pulyaevskiy]

Nice find, this would explain why I couldn't reproduce it on my end (different Flutter versions).

[stemuk]

@pulyaevskiy As of the latest flutter version (Channel dev, v0.10.0) the divider issue is fixed! The only issue remaining would be the ability to toggle the bold and italic style options on and off. 👍

[pulyaevskiy]

Awesome, thanks for confirming!
Please stay tuned for the remaining fix. I'm trying my best to get some time for this.

[kolja-esders]

Any update on the underlying issue of allowing toggling of bold/italic attributes @pulyaevskiy? Would be greatly appreciated!