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

Added ToolTip.ShowOnDisabled and ToolTip.ServiceEnabled #14928

Merged

Conversation

TomEdwardsEnscape
Copy link
Contributor

ToolTipService now processes raw mouse input, to allow tooltips on disabled controls. It is also now possible to disable the service entirely, to allow users to write their own logic for hiding and showing tooltips without interference from Avalonia.

What is the current behavior?

Tooltip behaviour is currently linked to pointer events on the host control. These events are not raised at all if the control is disabled, so tooltips cannot be automatically opened on the control. It is also not possible to block a tooltip on an enabled control from opening automatically.

How was the solution implemented

ToolTipService now subscribes to the InputManager.Process event. I could have had TopLevel pass the events directly to ToolTipService and avoid this dependency, but that would mean that tooltip processing would not occur until either after InputManager.PostProcess or before InputManager.PreProcess, both of which seem wrong.

In order to support this subscription in unit tests, where multiple applications are constructed, ToolTipService is disposable. This is not relevant to real-world applications because normally only one instance can exist in a given process.

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Fixes #3847

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the feature/tooltip-show-on-disabled branch from e99a0e5 to 0496a14 Compare March 13, 2024 08:26
@TomEdwardsEnscape
Copy link
Contributor Author

The ApiCompat tool is complaining that three hit test methods no longer exist. I'm not sure how to fix that, because they do. I have added overloads which take an optional bool parameter, but the original method signature is still there, now defined like this:

public static IInputElement? InputHitTest(this IInputElement element, Point p) => InputHitTest(element, p, true);

@Takoooooo
Copy link
Contributor

@TomEdwardsEnscape Since you've extended the API you probably need to re-run the APICompat. Usually it will run automatically when you build the project but you may run it manually also https://github.com/AvaloniaUI/Avalonia/wiki/ApiCompat---Diffs

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

LGTM in general though I think the disabling of the feature where the tooltip is hidden on pointer down was unintentional?

src/Avalonia.Controls/ToolTipService.cs Show resolved Hide resolved
src/Avalonia.Base/Input/InputExtensions.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/Input/InputExtensions.cs Show resolved Hide resolved
@MrJul
Copy link
Member

MrJul commented Mar 26, 2024

Nitpicking about naming: shouldn't these properties be IsShownOnDisabled and IsServiceEnabled?

ShowOnDisabled really looks like a method name, and Is is usually used as a boolean prefix, even if we have exceptions (yes, I'm looking at you, Focusable.)

@TomEdwardsEnscape
Copy link
Contributor Author

Nitpicking about naming: shouldn't these properties be IsShownOnDisabled and IsServiceEnabled?

The names are carried over from WPF, but have drifted a little because they really ought be properties of ToolTipService, but in Avalonia that is a private class.

WPF has these:

I don't know if it makes sense to follow the "IsState" pattern here because neither of these are really properties of the target object. They are configuring how the the service behaves when processing the target object.

But, I've not got any strong opinions here.

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the feature/tooltip-show-on-disabled branch 2 times, most recently from cb7c092 to daefe5c Compare March 27, 2024 07:43
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046647-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

ToolTipService now processes raw mouse input, to allow tooltips on disabled controls
Updated tests and control catalog
@TomEdwardsEnscape TomEdwardsEnscape force-pushed the feature/tooltip-show-on-disabled branch from daefe5c to f86870f Compare March 27, 2024 12:07
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046655-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

LGTM now. I think the naming makes sense, though I'll leave this open for a few days to get any further feedback in that area (it's Easter holidays anyway).

@MrJul
Copy link
Member

MrJul commented Mar 29, 2024

Regarding the naming, note that in .NET 4.8 there's also ToolTip.ShowsToolTipOnKeyboard, using the third person.
My preference would be ShowsOnDisabled for consistency, but either is fine.

ServiceEnabled on the other hand really feels like it misses that Is prefix, imo (like in WPF). And since there's no ToolTipService, it's hard to know what exactly "Service" refers to here. Ideally the property should be IsEnabled but that already exists, as you mentioned. What about ToolTip.IsToolTipEnabled?

No blockers here for me, just discussing, I know naming is hard :)

@TomEdwardsEnscape
Copy link
Contributor Author

What if ToolTip service were public and exposed same property names as WPF?

It would look like this pseudocode:

public static class ToolTipService
{
    ShowOnDisabledProperty
    IsEnabledProperty

    internal class Impl : IToolTipService { }
}

@TomEdwardsEnscape
Copy link
Contributor Author

@grokys @MrJul

@maxkatz6
Copy link
Member

maxkatz6 commented Apr 3, 2024

Adding ToolTipService would be a bit confusing, when we already have some WPF ToolTipService members kept in ToolTip instead.

@maxkatz6 maxkatz6 merged commit e947675 into AvaloniaUI:master Apr 6, 2024
10 checks passed
@maxkatz6 maxkatz6 added feature backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Apr 6, 2024
maxkatz6 pushed a commit that referenced this pull request Apr 6, 2024
ToolTipService now processes raw mouse input, to allow tooltips on disabled controls
Updated tests and control catalog
@TomEdwardsEnscape TomEdwardsEnscape deleted the feature/tooltip-show-on-disabled branch April 6, 2024 09:39
@maxkatz6 maxkatz6 added backported-11.1.x and removed backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display ToolTip on Control with IsEnabled=False
6 participants