Added more TextBox ContextMenu configuration #2758
Conversation
-Allow for disabling cut/copy/paste in a TextBox -Allow for custom context menu items in a TextBox -Added IsCutCopyPasteInContextMenu property (defaults to true) -Added ExtraContextMenuItems property (defaults to null)
| AddExtraItemsToContextMenu(tb, tb.ContextMenu.Items.Count > 0); | ||
| if (tb.ContextMenu.Items.Count == 0) | ||
| { | ||
| AddNoItemsAvailToContextMenu(tb); |
There was a problem hiding this comment.
Why should we add a menu with no function behind?
There was a problem hiding this comment.
Good point 😅 That was my gut reaction when an empty menu showed a small black square. I set it to hidden/visible to show/not show the menu as necessary.
There was a problem hiding this comment.
(let me know if you want me to rebase)
| @@ -53,6 +56,8 @@ public class TextBoxHelper | |||
| public static readonly DependencyProperty HasTextProperty = DependencyProperty.RegisterAttached("HasText", typeof (bool), typeof (TextBoxHelper), new FrameworkPropertyMetadata(false, FrameworkPropertyMetadataOptions.AffectsMeasure | FrameworkPropertyMetadataOptions.AffectsArrange | FrameworkPropertyMetadataOptions.AffectsRender)); | |||
|
|
|||
| public static readonly DependencyProperty IsSpellCheckContextMenuEnabledProperty = DependencyProperty.RegisterAttached("IsSpellCheckContextMenuEnabled", typeof(bool), typeof(TextBoxHelper), new FrameworkPropertyMetadata(false, UseSpellCheckContextMenuChanged)); | |||
| public static readonly DependencyProperty IsCutCopyPasteInContextMenuProperty = DependencyProperty.RegisterAttached("IsCutCopyPasteInContextMenu", typeof(bool), typeof(TextBoxHelper), new FrameworkPropertyMetadata(true, IsCutCopyPasteInContextMenuChanged)); | |||
There was a problem hiding this comment.
Isn't it better to rename it to UseDefaultContextMenu or AllowDefaultContextMenu...
/cc @thoemmi
There was a problem hiding this comment.
I like UseDefaultContextMenu much better than what I chose! I'll wait a day or two before changing it to wait to hear from @thoemmi .
Edit: Although UseDefaultContextMenu = False does imply that spelling corrections will be removed...should we differentiate between the spelling corrections and cut/copy/paste?
There was a problem hiding this comment.
I agree with @punker76 that the naming can be improved. However, I'd would negate it (generally I think boolean properties should be false by default and you have to switch 'em on explicitly) and name it ExcludeDefaultContextMenuItems.
| if (extraMenuItems != null && extraMenuItems.Count > 0) | ||
| { | ||
| // Add separator if necessary, then extra items to menu | ||
| if (shouldAddSeparator) |
There was a problem hiding this comment.
I think we should not add a default separator, cause users can't avoid this. If the user want it then he should do this itself.
There was a problem hiding this comment.
The List<MenuItem> is now a List<FrameworkElement>.
| throw new InvalidOperationException("The property 'IsCutCopyPasteInContextMenuChanged' may only be set on TextBoxBase elements."); | ||
| } | ||
|
|
||
| tb.SetValue(IsCutCopyPasteInContextMenuProperty, e.NewValue); |
There was a problem hiding this comment.
Is this really necessary? I'd think this method is called because the IsCutCopyPasteInContextMenu was changed, so setting it again is not needed. But I may be wrong.
There was a problem hiding this comment.
@thoemmi You're not wrong, this is not necessary. /cc @Deadpikle
| else | ||
| { | ||
| tb.SetValue(ExtraContextMenuItemsProperty, e.NewValue); | ||
| tb.ContextMenu = GetDefaultTextBoxBaseContextMenu(tb); |
There was a problem hiding this comment.
Updating the context menu in each and every XxxChanged method seems wrong to me. I'd think that creating the actual context menu inside TextBoxBaseContextMenuOpening should be sufficient. @punker76 ?
There was a problem hiding this comment.
@thoemmi Yes the opening event should be sufficient /cc @Deadpikle So the ExtraContextMenuItemsChanged is not necessary.
| @@ -53,6 +56,8 @@ public class TextBoxHelper | |||
| public static readonly DependencyProperty HasTextProperty = DependencyProperty.RegisterAttached("HasText", typeof (bool), typeof (TextBoxHelper), new FrameworkPropertyMetadata(false, FrameworkPropertyMetadataOptions.AffectsMeasure | FrameworkPropertyMetadataOptions.AffectsArrange | FrameworkPropertyMetadataOptions.AffectsRender)); | |||
|
|
|||
| public static readonly DependencyProperty IsSpellCheckContextMenuEnabledProperty = DependencyProperty.RegisterAttached("IsSpellCheckContextMenuEnabled", typeof(bool), typeof(TextBoxHelper), new FrameworkPropertyMetadata(false, UseSpellCheckContextMenuChanged)); | |||
| public static readonly DependencyProperty IsCutCopyPasteInContextMenuProperty = DependencyProperty.RegisterAttached("IsCutCopyPasteInContextMenu", typeof(bool), typeof(TextBoxHelper), new FrameworkPropertyMetadata(true, IsCutCopyPasteInContextMenuChanged)); | |||
There was a problem hiding this comment.
I agree with @punker76 that the naming can be improved. However, I'd would negate it (generally I think boolean properties should be false by default and you have to switch 'em on explicitly) and name it ExcludeDefaultContextMenuItems.
|
Ok, I see the point. There's already a `TextBoxBaseContextMenuOpening`, but
it's used only in `UseSpellCheckContextMenuChanged`. Adding the
registration to your new `XxxChanged` methods wouldn't help, because if you
would call `UseSpellCheckContextMenu="false"`, the event would get
unsubscribed.
As far as I can see now is that having all the `XxxChanged` are required.
However, move the creation of the `ContextMenu` to a single method, so it
can be called from several places.
…On Thu, Dec 8, 2016 at 3:30 PM, Deadpikle ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/MahApps.Metro/MahApps.Metro.Shared/Controls/Helper/TextBoxHelper.cs
<#2758>:
> + tb.ContextMenu = GetDefaultTextBoxBaseContextMenu(tb);
+ AddExtraItemsToContextMenu(tb);
+ tb.ContextMenu.Visibility = tb.ContextMenu.Items.Count > 0 ? Visibility.Visible : Visibility.Hidden;
+ }
+
+ private static void ExtraContextMenuItemsChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
+ {
+ var tb = d as TextBoxBase;
+ if (null == tb)
+ {
+ throw new InvalidOperationException("The property 'ExtraContextMenuItemsChanged' may only be set on TextBoxBase elements.");
+ }
+ else
+ {
+ tb.SetValue(ExtraContextMenuItemsProperty, e.NewValue);
+ tb.ContextMenu = GetDefaultTextBoxBaseContextMenu(tb);
@punker76 <https://github.com/punker76> @thoemmi
<https://github.com/thoemmi> I agree with putting all the menu setup in
one place, but how do you suggest I hook into the ContextMenuOpening
event without the XxxChanged calls? Can't do it via static class
EventManager call since TextBoxHelper does not derive from UIElement or
ContentElement.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2758>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEf2lmELJkpZcSlu4pq1Fsc_EmwehqGks5rGBRygaJpZM4LGA9l>
.
|
Only add ContextMenuOpening event if not already added
…to extra-textbox-menu
|
@Deadpikle Hey, thx for this changes and your time to do this. Sorry for my lateness. I review this and had some thoughts about this and the new attached properties. And I come to the conclusion that we don't need this new properties with some other changes... |
|
@punker76 It's cool; I'm glad there is a solution that will work for everyone and is a lot cleaner than what I did. :) |
What changed?
I'm not sure that properties were the best way to go about doing this, but I thought I'd do a pull request all the same. I'm open to better ways.