Fix missing update of mouse coordinates in MouseCoordinateWidget

[starnutoditopo]

At runtime, in Mapsui.Samples.Wpf, section "Editing", the MouseCoordinateWidget only updates sometimes (notably: when something else needs a UI refresh):

With this patch, the mouse position is updated every time it changes, by invoking Map.RefreshGraphics();.

[CLAassistant]

All committers have signed the CLA.

[charlenni]

Makes sense to me. Found the same today, but didn't investigate this. Thanks to @starnutoditopo for find this.

[pauldendulk]

Thanks for the fix! I am trying to think of a better way to refresh widgets without having to use INotifyPropertyChanged.

If I remember correctly you contributed to Mapsui a long long time ago. I can't find you in the issues. Perhaps it was BruTile or SharpMap.

[charlenni]

But why do you want to circumvent INotifyPropertyEvent? You don't want to have the Map in the Widgets. And you don't want to have a common event like INotifyPropertyEvent. I could then only think about a flag, where the Widgets sets to show, that the Map should be redrawn. Or is there something else?

[starnutoditopo]

[...]
If I remember correctly you contributed to Mapsui a long long time ago. I can't find you in the issues. Perhaps it was BruTile or SharpMap.

Hi, Paul! Yes: I think we exchanged some mails very long ago, but I don't remember for what project. Nice to "meet" you again!

[charlenni]

Should be dropped. Problem is, that MouseCoordinateWidget doesn't implement INotifyPropertyChanged and therefore doesn't update the map. If MouseCoordinateWidget did it correct, then it would work correct. This PR is a workaround. And as a side effect, we have to propagate the map to the widget.

Solution is to implement INotifyPropertyChanged for Widget. Could be found here.

[pauldendulk]

Solution is to implement INotifyPropertyChanged for Widget. Could be found here.

I am not going to accept INotifyPropertyChanged in the current phase. I want to investigate alternatives. It will take time to investigate alternatives. And I want to spend my time working on other issues the coming period.

In the meantime I will merge this. It is a small fix, it can easily be reverted in follow up PRs.

[charlenni]

@pauldendulk Is there a reason, why INotifyPropertyChanged interface is not a viable way?

[pauldendulk]

@pauldendulk Is there a reason, why INotifyPropertyChanged interface is not a viable way?

I was afraid you might ask this 😬 I did not want to answer because I tend to use an hour of my time by trying to formulate things like this. A short version of my arguments:

• First, my mind is not made up, it is very possible we may choose INotifyPropertyChanged, but I want to take my time to look for alternatives.
• INotifyPropertyChanged tends to get verbose. A single field becomes 6 lines of code.
• Ah, I know what you were thinking! We use some tool to generate them, like IL weaving or source generators. Sure, it will make the code more compact but it will make things more complex. The tools will cause bugs now and then. The build will break more often, and there will be ... things! If you have more stuff, more stuff breaks, that is a rule in life. (this still may be the best option though, we could look into how this works in Blazor).
• When introducing widgets for the very first time I mentioned that a risk is that it would develop into a UI framework. We should try to avoid rewriting Avalonia for a few overlays on our map. We should not add everything we know from those framework and write our own mini version of it. We should provide the minimal tools to make possible what we need and it should be extendable.
• Notification flows are often hard to debug, we had this problem in debugging some issues in the MapView, and I had this problem in many projects in the past.
• Notification flows can cause performance problems which are hard to track down. I call this a notification storm. One notification triggers another, or triggers notifications in a collection of items, all sending their own notifications.
• INotifyPropertyChanged allows for specific updates, just this field is changed, so update just this part of the UI. We do not use this in Mapsui. Our most important scenario is good performance while dragging and zooming. In that case we update every pixel in the map and widgets. We need do this in 16 ms. This is simple. So, we do not need the specificity of INotifyPropertyChanged, just a Refresh().
• Listeners to INotifyPropertyChanged can cause memory leaks.
• And now my biggest fear: When we implement INotifyPropertyChanged the next feature request will be to add binding. Which will quadruple the notification code and will make bugs even more untraceable. And will recreate all the MVVM misery that MAUI developers are suffering from.

Thanks for your question. It helpt my own thinking when writing this here.

[charlenni]

First, my mind is not made up, it is very possible we may choose INotifyPropertyChanged, but I want to take my time to look for alternatives.

We use it already.

INotifyPropertyChanged tends to get verbose. A single field becomes 6 lines of code.

Yes. But normally the same code. And the lines are easy, and all using C# understand what's going on.

Ah, I know what you were thinking! We use some tool to generate them, like IL weaving or source generators. Sure, it will make the code more compact but it will make things more complex. The tools will cause bugs now and then. The build will break more often, and there will be ... things! If you have more stuff, more stuff breaks, that is a rule in life. (this still may be the best option though, we could look into how this works in Blazor).

No, I know, that I shouldn't introduce any tools.

When introducing widgets for the very first time I mentioned that a risk is that it would develop into a UI framework. We should try to avoid rewriting Avalonia for a few overlays on our map. We should not add everything we know from those framework and write our own mini version of it. We should provide the minimal tools to make possible what we need and it should be extendable.

Your suggestion was to add a LayerListWidget. I want to have a LoggingWidget. I only introduced long time ago ScaleBarWidget, because I thought, that this is a useful improvement, and the ButtonWidget, because I needed it for the MapView. Sorry for that. The rest of this mess is not my work.

Notification flows are often hard to debug, we had this problem in debugging some issues in the MapView, and I had this problem in many projects in the past.

Yes, could be. But at the moment we have no better idea. And what the result is to wait, until we have a better idea, you could see at the Layers. We still have no possibility for ordering them.

Notification flows can cause performance problems which are hard to track down. I call this a notification storm. One notification triggers another, or triggers notifications in a collection of items, all sending their own notifications.

Yes, as above: Isn't it better to make a rework with INotifyPropertyChanged than to do nothing and keep it as it is (ScaleBarWidget use float for Height, TextBox use int? for Height and BoxWidget use int)? Isn't it better to make the code better and improve basics later?

INotifyPropertyChanged allows for specific updates, just this field is changed, so update just this part of the UI. We do not use this in Mapsui. Our most important scenario is good performance while dragging and zooming. In that case we update every pixel in the map and widgets. We need do this in 16 ms. This is simple. So, we do not need the specificity of INotifyPropertyChanged, just a Refresh().

Yes, as above: We could stop the whole development with this argument. Isn't it better to wait until we have the optimal code? And btw. you will not look into this new code, because it is a horrible mess of changes to the whole code, because you have to change even more.

Listeners to INotifyPropertyChanged can cause memory leaks.

Yes, as above: Could be, but not to improve the code could bring problems too.

And now my biggest fear: When we implement INotifyPropertyChanged the next feature request will be to add binding. Which will quadruple the notification code and will make bugs even more untraceable. And will recreate all the MVVM misery that MAUI developers are suffering from.

Could be. I will not introduce it. I only want to rearrange some code. I'm introducing nothing special to widgets. I make a rework of the code, use more inheritance for the different widgets, simplify the code for handling touchable widgets (add IWidgetTouchable and IWidgetExtended to one interface) and reorder the folder structure to make it more clear what is going on.

When I started with Mapsui.Forms, you said, I should make an extra project, because this brings too much trouble into the development of Mapsui. I never read this again from your side. Not at Avalonia 0.x and 11.x, Eto, WinUI or Uno. Sorry, forgot Blazor and Maui. None of this improved the functionality of Mapsui, but each of them brought trouble. And in between this there was .NET standard, .NET core, .NET 5, 6, 7 and 8, a change of the build process and much more. And now the change of the doc framework. Evolution of Mapsui? I know, that some of this has to be done, but only?

I could do nothing for Mapsui 5. It's all about build process and frameworks. Nothing, that I can help. The only I could do is to transfer MapView parts to MapControl. The first should be the buttons. But for this, it would be good to have GroupedWidgets class to add two or more buttons to one position. And for this, the widgets should be reworked, because there is a mess of different things that are not good. That is, what I did.

Have I done it wrong? Yes, totally. And in the end I realised that nobody can check this. That's why I'm doing it again. Step by step, so that you can follow every little commit. Will the end result be exactly the same as before? Sure, it can't be any different. Can I wait until the decision has been made whether to continue using INotifyPropertyChanged or not? No, completely impossible. In 3 months or 1 year, I can only track my changes with the same effort as you. So if I have to wait with the PRs until this decision has been made, then it is better if I delete them straight away. No problem with this.

Sorry, that I waste your time. I needed 2 days to make the first version and another 2 days to now make the PRs with the small steps. Perhaps you could at the end look one or two hours into this. That would be nice.

Thank you for your work.

[starnutoditopo]

😧
Sorry: it was not my intention to raise all such discussion!

[charlenni]

@starnutoditopo No problem. It's not about your PR, but fits here. So, you made all correct. Thank you.

[inforithmics]

Some thoughts about this discussion:

1. Listeners to INotifyPropertyChanged can cause memory leaks.
   This can be avoided by Implemented a WeakEvent Pattern, this means that the references in the Event are hold be WeakReferences, meaning that when the object isn't hold anywhere, it is collected.

private readonly WeakEventManager propertyChanged = new();

public event PropertyChangedEventHandler? PropertyChanged
{
    add { this.propertyChanged.AddEventHandler(value); }
    remove { this.propertyChanged.RemoveEventHandler(value); }
}

2. And now my biggest fear: When we implement INotifyPropertyChanged the next feature request will be to add binding.
   For me the Map and the widgets Layers are a kind of DOM that notify the MapControl of changes by events or state (dirty) And then this part of the map is rendered again (Because of the Timer). So this isn't used in the Designer WPF,MAUI Xaml or similar.
   Then this needs to be in the UI Control because it is platform specific.

3. Ah, I know what you were thinking! We use some tool to generate them, like IL weaving or source generators.
   I prefer to do this by hand with following pattern.

public string Property
{
    get => this.property;
    set => this.SetProperty(ref this.property, value);
}

protected void SetProperty<T>(ref T backingStore,T value,[CallerMemberName] string propertyName = "")
{
    if (EqualityComparer<T>.Default.Equals(backingStore, value))
    {
        return;
    }

    backingStore = value;
    this.OnPropertyChanged(propertyName);
}

Advantage is that isDirty Properties can although be filled by overriding the OnPropertyChanged Method.

[charlenni]

@inforithmics and @pauldendulk: See #2376