Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Discussion: Visibility enum vs. IsVisible bool #5206

Closed
robloo opened this issue Dec 21, 2020 · 15 comments
Closed

Discussion: Visibility enum vs. IsVisible bool #5206

robloo opened this issue Dec 21, 2020 · 15 comments

Comments

@robloo
Copy link
Contributor

robloo commented Dec 21, 2020

Is your feature request related to a problem? Please describe.

All other XAML-based frameworks (WPF, UWP, WinUI) use an enum to set the visibility of a control. In the interest of code-compatiblity, should Avalonia support this enum as well?

In WPF:

public Visibility UIElement.Visibility

public enum Visibility
{
  Visible,   // Display the element
  Hidden,    // Do not display the element, but reserve space for the element in layout.
  Collapsed  // Do not display the element, and do not reserve space for it in layout.
}

In UWP/WinUI:

public Visibility UIElement.Visibility

public enum Visibility
{
  Visible,
  Collapsed
}

As can be seen, UWP dropped support for the Hidden value. I would guess Hidden wasn't used much in real-world apps to warrant it being carried over from WPF. This means, in UWP, the property is equivalent to a bool which is what Avalonia has done from the beginning:

Avalonia:

public bool Control.IsVisible

Describe the solution you'd like

Control visibility, like DependencyProperty, is one of the subtle changes that creates significant headache using Avalonia from existing windows code. It necessitates creating a lot of conditional code in places where clean MVVM isn't already used (in that case only the converters change). It can affect nearly every part of the UI. It would be great to discuss some solutions:

  • Adding Control.Visiblity and the corresponding enum to Avalonia could be done in some proposed compatiblity layers using something like an attached property. This would mitigate concerns with having two properties that do the same thing in Avalonia itself.
  • IsVisible could be removed from Avalonia and replaced with the WinUI equivalent -- which is a lot of breaking changes for existing Avalonia code.
  • If IsVisible is removed from Avalonia as it exists now, it could be replaced with the get-only property of the same name from WPF UIElement.IsVisible. WPF's property tells applications if a control is actually visible in the UI. This, along with its corresponding event, is especially useful to tracks when items are scrolled out of view for example.

Describe alternatives you've considered

The alternative is simply to use Avalonia's existing convention.

Additional context

Originally discussed here but broken up into separate issue.

@robloo
Copy link
Contributor Author

robloo commented Dec 21, 2020

@grokys Commented here

I don't know how deep we could go with this, for example I don't really want to support both the Visibility enum and IsVisible property as that would be confusing. Maybe some concept of "attached properties" in XAML would help here in XAML but that's not going to help in code until C# adds attached properties.

@grokys
Copy link
Member

grokys commented Dec 21, 2020

WPF's IsVisible is a PITA because it means that you can't simply bind to a boolean, even though most of the time you have one. In addition, Avalonia has the ! binding operator which makes it easy to manipulate booleans. In UWP it's even more annoying because Visibility is a boolean state.

So I think Avalonia's choice to use a bool IsVisible property is the right one. What remains is the compatibility problem. Regarding the alternatives you mentioned:

  • An "attached property": would be ideal, but this means we'd have to design and implement that feature.
  • Removing IsVisible I don't like for the reason you mention and the reasons outlined above

Another alternative is to have both IsVisible and Visibility. The problem with that is potential user confusion and deciding what happens if both get set.

@robloo
Copy link
Contributor Author

robloo commented Dec 21, 2020

WPF's IsVisible is a PITA because it means that you can't simply bind to a boolean, even though most of the time you have one. In addition, Avalonia has the ! binding operator which makes it easy to manipulate booleans. In UWP it's even more annoying because Visibility is a boolean state.

Agree with the sentiment. I guess the first question is: should Hidden ever be supported? The answer everyone has decided on in the past 10 years is no, it's unnecessary. This means there is no reason it MUST be an enum as it is only two logical states -- so agree with you again.

Using the enum does add some complexity but BoolToVisiblity is a standard converter these days. Since the Visibility in UWP IS a boolean state as you mentioned it is also possible to automatically map to a bool during XAML compilation and remove the need for a converter.

So I think Avalonia's choice to use a bool IsVisible property is the right one. What remains is the compatibility problem. Regarding the alternatives you mentioned:
Removing IsVisible I don't like for the reason you mention and the reasons outlined above

An additional problem aside from a boolean is the name of the property itself. What is Avalonia's plan for an equivalent property to WPF's IsVisible? That is a point of confusion as well for those coming over from that framework. A property of the same name does something different. It is also a missing feature in Avalonia.

Another alternative is to have both IsVisible and Visibility. The problem with that is potential user confusion and deciding what happens if both get set.

Another idea I heard was to add both, but mark Visibility as [Obsolete]. In terms of what happens if both get set, this isn't possible I think? The UI and dependency property system is single threaded and the last property set will apply. Perhaps you mean the circular dependency problem?

@grokys
Copy link
Member

grokys commented Dec 21, 2020

An additional problem aside from a boolean is the name of the property itself. What is Avalonia's plan for an equivalent property to WPF's IsVisible?

UIElement.IsVisible isn't present in UWP and personally I've never needed it in WPF, so I don't think it's that important.

The UI and dependency property system is single threaded and the last property set will apply.

IsVisible is a styled property and Visibility probably would have to be as well. This means that the properties have priorities. Combining priorities with two properties that do the same thing could be tricky.

@robloo
Copy link
Contributor Author

robloo commented Dec 21, 2020

UIElement.IsVisible isn't present in UWP and personally I've never needed it in WPF, so I don't think it's that important.

It has been requested to add this to WinUI microsoft/microsoft-ui-xaml#674. A lot of more advanced cases could use this (especially considering x:Load isn't in Avalonia right now). Granted the cases where it's needed are not simple apps but places where performance is critical. I wouldn't underestimate the long-term need of an equivalent for this in Avalonia based on personal need. Edit: Although another simply case this could be used is implementing 'ScrollIntoView' functionality in customized UI scenarios.

  • WPF has always had a read-only bool UIElement.IsVisible link and an event IsVisibleChanged link that allowed control developers to effectively stop/pause unnecessary work if a control isn't visibly active. Like stop an animation, pause DirectX rendering etc, and not waste battery at that point in time.

IsVisible is a styled property and Visibility probably would have to be as well. This means that the properties have priorities. Combining priorities with two properties that do the same thing could be tricky.

Interesting, I am not familiar with this concept and don't believe it exists in WPF/UWP. If there is documentation on how this works I would be curious to read up on it.

That said, it seems it could be handled by simply making the existing IsVisible a higher priority than Visibility. Then put in the docs that Visibility is only for backwards compatiblity and developers are strongly urged not to mix the two in code.

@maxkatz6
Copy link
Member

@robloo how x:Load is related to WPF.IsVisible? First is about control presence in the visual tree, and second is more about rendering state (is it actually rendered or not).
Indeed, x:Load is important for performance critical applications, where you can delay loading huge parts of the visual tree, or reuse complex components that might manage their children according to the input parameters (so x:Load should also work with bindings in ideal).
I am not sure where WPF.IsVisible may help in similar way. First point that I see is lists, but virtualization should be used there. In my personal experience I have a need for that property only for resize images lazily, when they are actually rendered, but that problem was solved with proper lists virtualization. Alternatively it could be solved with better Image control implementation.

Anyway, WPF naming here isn't really ideal either. In the WinUI discussion there are better proposals such as IsCurrentlyRendered.

@maxkatz6
Copy link
Member

Also if x:Load can replace WPF.IsVisible in some cases, it should be easier to implement (assuming that we would like to avoid breaking changes with IsVisible/Visibility props).

@robloo
Copy link
Contributor Author

robloo commented Dec 21, 2020

how x:Load is related to WPF.IsVisible?

Perhaps I didn't word that very well. As you said, x:Load cannot replace IsVisible in most cases. However, it is useful to lazily load the UI. This can be used in place of the VisibilityChanged event and IsVisible=true check before starting up some more performance-intensive parts of the UI. Again, this is relevant in cases like scrolling or when switching between views. When you discover you need this type of performance optimization you really need it. Granted most apps aren't doing something this intensive.

@robloo
Copy link
Contributor Author

robloo commented Dec 21, 2020

Anyway, WPF naming here isn't really ideal either. In the WinUI discussion there are better proposals such as IsCurrentlyRendered.

Agree a better name could be found. Compatiblity would then be a concern but much less of one since this property is used much less in actual code.

@robloo
Copy link
Contributor Author

robloo commented Dec 21, 2020

Also an interesting quote from one of Microsoft's own devs that worked on WPF I believe and is still involved in WinUI. Just some history on Hidden that validates a two-state bool property is actually the best decision.

WPF's Visibility has three states to match the HTML behavior of both display:none (Visibility.Collapsed) and visibility:hidden (Visibility.Hidden). Visibility.Hidden has generally been considered a mistake because you can get the same thing by setting the Opacity to 0, and because the enum is more difficult to work with than a bool.
microsoft/microsoft-ui-xaml#674 (comment)

@kekekeks
Copy link
Member

We might want to somehow extend the property system to have "proxy" properties that aren't don't have their own storage and just map to another property with converter applied. If we've had something like that, adding an [Obsolete] Visibility property would be trivial.

@workgroupengineering
Copy link
Contributor

You could use corce callback.

@robloo
Copy link
Contributor Author

robloo commented Dec 22, 2020

We might want to somehow extend the property system to have "proxy" properties that aren't don't have their own storage and just map to another property with converter applied. If we've had something like that, adding an [Obsolete] Visibility property would be trivial.

This would definitely be the safest approach. Managing a property like this without a single source of truth can open up a lot of unforeseen problems or complications. This idea may also be general-purpose enough to use in other scenarios not yet discussed. I'm not sure if anyone has an idea in mind of how to implement this type of proxy property though?

Edit: Callbacks/actions similar to coercion may not be a bad idea. It should be separate from coercion though. We could consider something like a DataProvider from UWP's drag/drop. (It's similar but not exactly what we are looking for, if nothing more I like the name). Anyway, this would likely be useful for a number of other cases as well.

A DataProvider would need to define a SetValue action and GetValue action (possibly consider supporting something like IValueConverter as well). If a DataProvider exists for a property, those actions are run to get/set the value instead of using the framework's systems. These actions are used even during binding. Aside from that, it would be quite general purpose. It could forward to another property if needed or even get data from another location.

@robloo
Copy link
Contributor Author

robloo commented Dec 22, 2020

Just an idea below. An IValueProvider could be provided during construction of a PropertyMetadata class within a Styled/DirectProperty just like a coercion callback is.

Also, coercion should apply:

  1. Before SetValue is invoked
  2. After GetValue is invoked
public interface IValueProvider<T>
{
  public T GetValue(...);
  public void SetValue(T value, ...);
}

// Default implementation
public class ValueProvider<T> : IValueProvider<T>
{
  public Action<T> SetValueAction { get; set; }
  public Func<T> GetValueAction { get; set; }

  public T GetValue()
  {
    return (GetValueAction != null ? GetValueAction.Invoke() : default(T));
  }

  public void SetValue(T value)
  {
    SetValueAction?.Invoke(value);
  }
}

The above is just an idea to keep the discussion moving. Changing the signatures to something more like coercion has right now should be considered.

@robloo
Copy link
Contributor Author

robloo commented Mar 2, 2021

Would someone be kind enough to move this to Discussions?

@maxkatz6 maxkatz6 closed this as completed Mar 2, 2021
@AvaloniaUI AvaloniaUI locked and limited conversation to collaborators Mar 2, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants