TextToolbar Control for Editing RichEditBox #1116

WilliamABradley · 2017-04-21T14:37:13Z

TextToolbar w/ MarkDown & RichText Support from Markdown Text Editor #1107.
Shifted from Old branch where I had made a mess due to overwriting my other PR, you can find the original PR and commits at TextToolbar for RichEditBox #1109.
Currently has a Sample Page and a basic RichTextFormatter Class that could be populated further by whoever is willing to help make this a Universal TextToolbar.

…ould now not overwrite my other PR.

dnfclas · 2017-04-21T14:37:17Z

@WilliamABradley,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

skendrot · 2017-04-21T17:34:36Z

Things to remember with formatting is it's more than just formatting selection. If I hit the bold button or Ctrl-B it should make all of the new text I type bold until I turn it off. Is this handled?

skendrot · 2017-04-21T17:28:12Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TextToolbar/TextToolbarPage.xaml.cs

+        }
+
+        private void EditZone_TextChanged(object sender, RoutedEventArgs e)
+        {


No need to change the preview text every time the text changes, only need to change when the pivot changes

@skendrot fair enough

skendrot · 2017-04-21T17:37:28Z

Microsoft.Toolkit.Uwp.UI.Controls/TextToolbar/Formats/Format.cs

+
+namespace Microsoft.Toolkit.Uwp.UI.Controls.TextToolbarFormats
+{
+    public enum Format


I don't think tying the control to these types makes sense. I would rather see it fire events for the buttons like BoldActivated/BoldDeactivated or FormatActivated event with a Type parameter of "Bold". Then controls could be updated to support the toolbar. Of course we cannot change the RichEditBox, so maybe it can support that out-of-the box

@skendrot the Reason for the Formatter class, is that different formats require different sets of buttons. E.g. RichText supports Underline, while Markdown doesn't. Therefore, RichText has an underline button and Markdown doesn't. This also allows users to define their own Formatter class with their own styling and Default Button map.

Perhaps the abstract methods can be removed, and attached to button directly, when someone adds a CommonButton to their ButtonMap?

Understood. All the more reason not to tie the toolbar to any particular controls and instead allow for buttons to removed and/or added and fire events when they are activated

That would mean that the user would have to add their own code to achieve the same functionality, which would defeat the point of having a control that is universal as per discussion at #1107. If not using a formatter class, how do you propose supporting both Rich Text and Markdown? With what you are saying, the user might as well just use a CommandBar and create their own from scratch.

There isn't a way to create one control to be "universal" for any text entry. What happens if I want this toolbar for a regular Textbox and not a RichTextbox? TextBox still supports bold, italics, hyperlinks, colors, etc.

Having code paths like

if(Format == RichText) { DoRichText(); } else if(Format == Markdown) { DoMarkdown(); } else if(Format == Textbox) { DoTextBox() } else { DoSomethingElse(); }

Is a perfect example of violating the Open/Closed principle of software development.

If I'm the only person who thinks it should be "more generic" then throw my suggestion away

If you actually look at the formatter implementation, you will see that my code isn't like that. If a user wants to implement their own experience, they can either inherit the Formatter class, defining their own Default buttons, how to deal with Bold, etc. They can also insert additional custom buttons via XAML or code behind, remove default buttons via XAML or code behind.

The only purpose the Format enum has, is to Instantiate the RichText Formatter or the Markdown formatter. Set it to Custom, and then you can provide it your own inherited Formatter.

I have ensured that are plenty of places for the user to be able to adapt the code to suit their needs.

It is generic, yet you can still achieve similar functionality.

WilliamABradley · 2017-04-22T00:23:16Z

@skendrot This was just the initial functionality I was setting up, considering things such as Shortcut keys, after the initial code was setup.

Again, Shortcut keys would probably be of better use in the Formatter class, so that Different default Buttons can receive their own key. Such as the Code Button, which wouldn't exist in the Rich Text version of the Toolbar, could get CTRL + "[", or something like that.

Also if you test in the GitHub Comments section, which I have tested against for implementation, you will see that Ctrl-B only changes selection, and pressing it again will remove the Bold formatting, as if the button was pressed.

-Added ShiftActivation Action to ToolbarButton, for holding shift when clicking or doing a keyboard combination. Made new Action called Click to hide the AppBarButton Click Handler, allowing for cleaner code, and a way to activate ShiftActivation when Shift-Clicking, such as pressing CTRL + SHIFT + K will add the Link template, instead of Opening the Link Creator. -Added Code Block support, now just have to implement Inline Code Selection, however, It requires a check to see if it is a single line selection. -You no longer have to manually iterate the ListLineIterator, it also starts at 1. -Added bool to determine if the ListLineIterator has reached the last line. -Added option to Wrap selected Lines for a list, with Empty Lines (For Line Chars). -Added Logic for ObservableCollections in ButtonMaps, and Remove Buttons, so that Buttons can be dynamically added and removed at anytime during operation. -Moved simple methods out of TextToolbar.Events into TextToolbar.Methods.

WilliamABradley · 2017-04-23T15:21:16Z

@skendrot I have implemented Rudimentary Shortcut keys, however, I need to use reflection to disable the RichEditBox's internal KeyDown interactions, so that Markdown can use combinations such as CTRL + R & CTRL + L, as it doesn't support Right to Left Text, Or I could just stray away from using those keys.

I still like having a Formatter class to format the text for specific formats like Markdown, as RichEditBox, doesn't have the support built-in, but the code split could be moved, so two Controls can be made, an Inherited RichEditBox that uses the Formatter Class, and then the TextToolbar could attach to this Inherited RichEditBox, and trigger it's methods. Which is a more natural location for a Formatter Style class, however, doing this manipulation in the TextToolbar is a simpler implementation, and it is easier for a user to Implement.

-TextToolbar now properly updates the XAML Designer, as the Dependency property are handled with PropertyChanged Events. (If Project code is disabled, nothing shows). -Added ToolbarButton as a parameter to every Formatter method, in order to do things such as enabling Toggling, and to handle the ToggleEnded Event. -Added Headers menu for MarkDown Formatter, to create different Headers. -Added Inline Check for Code Formatting. -All MD SetList methods are now Toggleable, so that they automatically add the List Char after going to a new line. -Removed Dependency on FontAwesome, by Designing List Icons using XAML. -Fixed Sample Page, Fleshed out Page, and added Binded Properties, so that users can see both the Markdown Formatter and RichText Formatter, and what it would look like in the dark theme. -Sample Page also demonstrates Adding/Removing Default Buttons, Custom Buttons with Keyboard shortcuts. -Added some basic implementation or RichText Formatting.

…on Action.

…t of translatable/replacable strings. -Fixed Formatter not loading Default Buttons into the TextToolbar Default Buttons map. Meaning that Default buttons can now be manually be modified before the UI appears.

…ate. -Added Back IsEnabled Property to Sample Page. -Added DependencyPropertyWatcher, to track changes in Dependency Properties.

nmetulev · 2017-04-25T05:31:11Z

Love the control. I'm slowly reviewing the PR. In the meantime, I keep running into a marshaling exception that you should probably take a look. It happens almost every time I play with the markdown format, switch to rich format and click on to view the xaml code, (or go back between xaml code and properties)

System.Exception occurred
  HResult=0x8001010E
  Message=The application called an interface that was marshalled for a different thread. (Exception from HRESULT: 0x8001010E (RPC_E_WRONG_THREAD))
  Source=<Cannot evaluate the exception source>
  StackTrace:
   at Windows.UI.Xaml.DependencyObject.ClearValue(DependencyProperty dp)
   at Microsoft.Toolkit.Uwp.UI.Helpers.DependencyPropertyWatcher`1.Dispose() in C:\Users\nikola\source\UWPCommunityToolkit\Microsoft.Toolkit.Uwp.UI\Helpers\DependencyPropertyWatcher.cs:line 79
   at Microsoft.Toolkit.Uwp.UI.Controls.TextToolbarSymbols.Symbol.Finalize() in C:\Users\nikola\source\UWPCommunityToolkit\Microsoft.Toolkit.Uwp.UI.Controls\TextToolbar\Symbols\Symbol.cs:line 56

nmetulev · 2017-04-25T05:44:21Z

We should not be changing the behavior of REB, so CTRL+R/L, should continue to support right to left. Strike-through does not usually have a keyboard shortcut, so we can leave it without, or assign some other key.

It seems like you've implemented quite a bit of the rich text formatter, why not finish it ;)

nmetulev · 2017-04-25T06:06:08Z

Please take a look at keyboard/controller navigation as well. Tab is working, but it looks like separators are also capturing focus. The controller doesn't work at all, the entire toolbar captures focus instead.

WilliamABradley · 2017-04-25T06:23:14Z

@nmetulev Whoops, I had only just added the DependencyWatcher in order to grey out the Custom List Icons. I'll fix that right away, I'll patch the separators capturing focus too. As for Controller Navigation, I can't imagine in it's current state that this control would be very helpful anyway.

I would finish Implementing RTF, however, I just don't have time. I have 5 days till Uni starts again, and I want to finish updating my app, TVPLAN. :)

…e previous button. -Hopefully fixed the Threading Exception for disposal of DependencyWatcher in the Symbol deconstructor. -Disabled Tab Stops around the place. The Toolbar is now more Keyboard/Controller friendly, requiring no confusing middle tabs. -Tooltip now fills out the Automation.Name field. -Separators shouldn't steal focus anymore.

WilliamABradley · 2017-04-25T07:01:21Z

@nmetulev All issues fixed in my latest Commit. I even threw in Automation.Name for the Toolbar Buttons.

…Continuous Integration :( -Removed Shortcut key for Strikethrough. -Fixed Automation.Name crash if Tooltip is null, e.g. switching Formatters.

skendrot · 2017-04-26T14:11:14Z

@WilliamABradley I'm not too worried about shortcuts keys, more that the toolbar handles more than changing selection. When a control uses this toolbar, and a tool is activated like Bold, then any new text entered from the location that bold was activated becomes bold.

WilliamABradley · 2017-04-26T14:30:56Z

@skendrot That experience is achieved when using Rich Text format on the TextToolbar, however, due to the design of Markdown formatting, it is not as feasible.

nmetulev · 2017-08-20T20:22:49Z

Thanks, I'll try it out. BTW, you should be able to grab the nugets from appveyor as well, in the artifacts section. Here are the ones from the last commit.

WilliamABradley · 2017-08-20T20:27:55Z

I feel silly now, Its because they had a pre-release flag on them. :(

WilliamABradley · 2017-08-20T20:48:48Z

Ok, it works without a specified Format now, but I seem to have lost Design Time viewing.

WilliamABradley · 2017-08-20T22:10:59Z

@nmetulev I fixed Design Time, it should all be working now :)

Currently Hyperlinking with Rich Text, ApplyDisplayUpdates don't seem to update the UI, it will return plain text, however, if you try to Open the Link creator on this text, it will turn blue in the Link Creator in the Label Field.

One fix for this would be to Copy the whole RTF Document to a string, and then restore it (This might take time on a big document).
Otherwise, I'm not sure what is stopping it from making the link blue.

…causing issues.

nmetulev · 2017-08-20T23:25:17Z

I noticed that about the link, the link still seems to work though. It seems like a Document bug. I'd recommend creating an issue for us to track once this PR is merged.

I have another request for you. I was playing with the API and the RemoveDefaultButtons property doesn't seem natural, kinda counterintuitive to add in order to remove. Wondering if there is another way? Maybe a property on the button. Kinda like Toolbar.DefaultButtons.GetButton(type)?.IsVisible = false; Just an idea. Thoughts?

WilliamABradley · 2017-08-20T23:44:53Z

Yea that makes more sense, the sole purpose, was that it allowed you to remove buttons in XAML. The same effect can probably be achieved this way.

… to change the visibility of a default button from XAML, with IsVisible. -Added method to get Default Button instance from TextToolbar in Enum. -Made RemoveToolbarItem public. -Updated Doc.

WilliamABradley · 2017-08-21T01:20:58Z

@nmetulev I have made the changes,
Now you can get a Default Button from the Toolbar by using Toolbar.GetDefaultButton(ButtonType), which returns the ToolbarButton Instance (Based on AppBarButton). So you can set Visibility to Collapsed.

nmetulev · 2017-08-21T06:18:57Z

That works much better :)

What is the purpose of ButtonModifications?

WilliamABradley · 2017-08-21T06:25:22Z

@nmetulev It allows you to do the same thing from XAML.

<controls:TextToolbar x:Name="Toolbar" Editor="{x:Bind Editor}">
    <controls:TextToolbar.ButtonModifications>
        <buttons:DefaultButton Type="Headers" IsVisible="False"/>
    </controls:TextToolbar.ButtonModifications>

and

var button = Toolbar.GetDefaultButton(ButtonType.Headers);
button.Visibility = Windows.UI.Xaml.Visibility.Collapsed;

Are Equivalent, XAML reflects it in the designer too.

WilliamABradley · 2017-08-21T06:34:07Z

So now we need a second review right?

nmetulev · 2017-08-21T06:35:37Z

Yeah, we need two approvals before we merge

WilliamABradley · 2017-08-21T06:36:49Z

I'm not sure if @skendrot is available to do this, is there someone else?

shenchauhan · 2017-08-21T20:01:53Z

I'll start reviewing this :)

shenchauhan

Love this control! Thanks for taking the time to build this. I have requested some changes which would be great to put in. Happy to discuss further if you disagree with anything.

shenchauhan · 2017-08-21T20:08:10Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TextToolbar/IsFormat.cs

+    {
+        public object Convert(object value, Type targetType, object parameter, string language)
+        {
+            return (Format)value == Format;


You could get an invalid cast exception here. Add some defense against that.

This was a quick converter in the Sample App, but I could make it available for use with the toolbar.

shenchauhan · 2017-08-21T20:10:00Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TextToolbar/IsFormat.cs

+
+        public object ConvertBack(object value, Type targetType, object parameter, string language)
+        {
+            throw new NotImplementedException();


Is there a chance this code could ever get called? Would people want to write back to a backing property? If so I'd add the convert back code too.

You can't exactly infer the type of Format from a Boolean, it Could RichText, Markdown or Custom

shenchauhan · 2017-08-21T20:24:16Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TextToolbar/TextToolbarCode.bind

+        var linkOpen = Task.Run(() => Launcher.LaunchUriAsync(new Uri(e.Link)));
+    }
+    catch
+    {


I'm pretty sure this will not be reachable. Check out : https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

The link works, do you mean the catch if the link isn't a valid Uri?

yup, as it was running in the Task.

shenchauhan · 2017-08-21T20:47:06Z

Microsoft.Toolkit.Uwp.UI.Controls/TextToolbar/Formats/RichText/RichTextButtonActions.cs

+        public void FormatUnderline(ToolbarButton button)
+        {
+            var format = Formatter.SelectionFormat;
+            if (!button.IsToggled)


A lot of repetitive logic. Consider a high order function.- not a must though, just makes reading easier.

Some of the properties on these a very slightly different though, such as Underline and list types.

shenchauhan · 2017-08-21T20:53:56Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TextToolbar/TextToolbarPage.xaml.cs

+                }
+            };
+
+            Toolbar.CustomButtons.Add(demoButton);


I found a bug:

Switch to dark

IsEnabled = Off

Format: RichText

Click Add Custom Button
Bullet points and numbering icons change colour

shenchauhan · 2017-08-21T20:55:12Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TextToolbar/TextToolbarPage.xaml.cs

+                }
+            };
+
+            Toolbar.CustomButtons.Add(demoButton);


If the toolbar is wider than the parent hosting it, it would be great to have something that indicates there is more.

Isn't that what the scrollbar is for? Something like that sounds difficult to implement.

…from SampleApp to be usable for anyone. Added safer conversion and ConvertBack. -Fixed List Button colors when IsEnabled=false, and changing the theme. -Moved Uri creation out of Task for Previewer.

WilliamABradley · 2017-08-21T21:55:58Z

@shenchauhan I have fixed the bug with the List Icons, the Link creation, and the Value converter, perhaps the More buttons indicator could be something to be added in a future PR?

shenchauhan · 2017-08-21T23:38:09Z

@WilliamABradley Agreed, approved and merging!! Nice work! 👍

nmetulev · 2017-08-22T01:23:24Z

Congrats @WilliamABradley 🥇

WilliamABradley added 2 commits April 22, 2017 02:23

-Created new Branch from Microsoft/Dev, as things are a mess.

4ecd97b

-Shifted TextToolbar code to fresh Branch off of Microsoft/dev, it sh…

e0642f6

…ould now not overwrite my other PR.

dnfclas added the cla-already-signed label Apr 21, 2017

skendrot reviewed Apr 21, 2017

View reviewed changes

WilliamABradley added 7 commits April 25, 2017 10:23

-Fixed Toggle issue for Code Block.

4066e5d

-Fixed SetSelection adding a gap to the closing selection.

9d5dad4

-Renamed Click Action to Activation, to be in line with ShiftActivati…

28a2dcf

…on Action.

-Added Event for Shortcut keys for External Use.

d42f443

-Moved Labels to TextToolbarStrings class, allowing for a mass transi…

0cbf5d0

…t of translatable/replacable strings. -Fixed Formatter not loading Default Buttons into the TextToolbar Default Buttons map. Meaning that Default buttons can now be manually be modified before the UI appears.

-Custom TextToolbar Symbols now react to IsEnabled with a Gray ViewSt…

ab51a1d

…ate. -Added Back IsEnabled Property to Sample Page. -Added DependencyPropertyWatcher, to track changes in Dependency Properties.

This was referenced Apr 25, 2017

MarkdownTextBlock doesn't support Multi-Line Formatting #1125

Closed

Created RoundImageEx, Moved ImageEx Core Logic to Base Class #1100

Merged

WilliamABradley added 3 commits April 27, 2017 01:42

-Removed all instances of C# 7 features in code, in order to support …

212d83a

…Continuous Integration :( -Removed Shortcut key for Strikethrough. -Fixed Automation.Name crash if Tooltip is null, e.g. switching Formatters.

-Added Header to DependencyPropertyWatcher.

c4e3089

-Removed another C# 7 feature instance.

3a6a48d

-Removed Local Functions.

0970fde

Fixed Buttons not appearing if Format wasn't specified.

2f354a0

WilliamABradley added 2 commits August 21, 2017 09:25

Attempts to fix Designer woes.

bd7861e

Fixed Design Time, removed Old Toggle Event Property on ToolbarButton.

f145541

Reverted SetList to EnsureAtNewLine, as it was erasing the line, and …

de9d492

…causing issues.

-Replaced RemoveDefaultButtons with ButtonModifications, allowing you…

5ff207c

… to change the visibility of a default button from XAML, with IsVisible. -Added method to get Default Button instance from TextToolbar in Enum. -Made RemoveToolbarItem public. -Updated Doc.

Fixed DefaultButtonModifications Change Event, Dependency Property Name.

e4b3d07

nmetulev approved these changes Aug 21, 2017

View reviewed changes

shenchauhan requested changes Aug 21, 2017

View reviewed changes

-Renamed IsFormat to ToolbarFormatActiveConverter, moved to Controls …

0d945a4

…from SampleApp to be usable for anyone. Added safer conversion and ConvertBack. -Fixed List Button colors when IsEnabled=false, and changing the theme. -Moved Uri creation out of Task for Previewer.

shenchauhan approved these changes Aug 21, 2017

View reviewed changes

shenchauhan merged commit 241575f into CommunityToolkit:dev Aug 21, 2017

WilliamABradley deleted the TextToolbar2 branch August 21, 2017 23:50

WilliamABradley mentioned this pull request Aug 22, 2017

Future Plans for the TextToolbar #1399

Closed

nmetulev added the controls 🎛️ label Aug 27, 2017

TextToolbar Control for Editing RichEditBox #1116

TextToolbar Control for Editing RichEditBox #1116

Uh oh!

Conversation

WilliamABradley commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Apr 21, 2017

Uh oh!

skendrot commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skendrot Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamABradley commented Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamABradley commented Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmetulev commented Apr 25, 2017

Uh oh!

nmetulev commented Apr 25, 2017

Uh oh!

nmetulev commented Apr 25, 2017

Uh oh!

WilliamABradley commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamABradley commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skendrot commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamABradley commented Apr 26, 2017

Uh oh!

nmetulev commented Aug 20, 2017

Uh oh!

WilliamABradley commented Aug 20, 2017

Uh oh!

WilliamABradley commented Aug 20, 2017

Uh oh!

WilliamABradley commented Aug 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmetulev commented Aug 20, 2017

Uh oh!

WilliamABradley commented Aug 20, 2017

Uh oh!

WilliamABradley commented Aug 21, 2017

Uh oh!

nmetulev commented Aug 21, 2017

Uh oh!

WilliamABradley commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamABradley commented Aug 21, 2017

Uh oh!

nmetulev commented Aug 21, 2017

Uh oh!

WilliamABradley commented Aug 21, 2017

Uh oh!

shenchauhan commented Aug 21, 2017

Uh oh!

WilliamABradley commented Apr 21, 2017 •

edited

Loading

skendrot Apr 26, 2017 •

edited

Loading

WilliamABradley commented Apr 22, 2017 •

edited

Loading

WilliamABradley commented Apr 23, 2017 •

edited

Loading

WilliamABradley commented Apr 25, 2017 •

edited

Loading

WilliamABradley commented Apr 25, 2017 •

edited

Loading

skendrot commented Apr 26, 2017 •

edited

Loading

WilliamABradley commented Aug 20, 2017 •

edited

Loading

WilliamABradley commented Aug 21, 2017 •

edited

Loading