RELOAD!

[CodeReviewCommunity/CodeReviewCommunity.github.io] 1 closed issue. 1 issue comment.

[rubberduck-vba/Rubberduck] 3 issue comments.

[Zomis/flexgame-server] 5 commits. 103 additions. 5 deletions.

[rubberduck-vba/Rubberduck] MDoerner commented on issue #3555: Set up a .settings file

> The way I see this, having the default hotkeys in the settings file allows to completely remove the hotkeys from the commands. Then only the hotkey bindings know about commands but commands have no idea about the existence of hotkeys.

[rubberduck-vba/Rubberduck] MDoerner commented on issue #3556: Parser errors aren't consistently reported as such

[rubberduck-vba/Rubberduck] MDoerner added label performance to issue #3556: Parser errors aren't consistently reported as such

@Duga IIRC, in one of the update notes for Antlr, they mentioned that the performance for left-recursion has been improved. We might want to try to parse OP's code again when we finally succeed in upgrading Antlr.

[rubberduck-vba/Rubberduck] inDeev commented on issue #3556: Parser errors aren't consistently reported as such

man

hmm ...

[rubberduck-vba/Rubberduck] rkapka commented on issue #3555: Set up a .settings file

@Duga regarding #3556, that code is not functionaly equivalent to the original code.

[rubberduck-vba/Rubberduck] rkapka commented on issue #3555: Set up a .settings file

> @MDoerner The latest version of the PR I mentioned works just like this. When creating the default hotkeys, a command is passed to the hotkey and the command itself knows nothing about the hotkey he's been bound to. But in order to get the collection of commands available for binding, I inject it into DefaultHotkeys. This causes an exception, because DefaultHotkeys is injected into HotkeyConfigProvider, which itself is injected into something else etc. In the PR's opening post I have des

[rubberduck-vba/Rubberduck] Vogel612 commented on issue #3555: Set up a .settings file

[rubberduck-vba/Rubberduck] Vogel612 commented on issue #3555: Set up a .settings file

The problem is that severl commands know about the presenter of view they are accessible from whose view model in turn gets the hotkeysettings injected.

actually the problem is that DefaultHotkeys needs an IEnumerable<CommandBase>, which depends on HotkeySettings provider, because the Settings command transitively depends on that

which results in that circularity

As rKakpa pointed out, that is not the circular dependency he got.

He tried to inject the IEnumerable<CommandBase> into the settings provider and ended up with the circular dependency above.

The particular problem he described is that the TestExplorerViewModel takes an IGeneralConfigProvider, which always also contains the HotkeySettingProvider.

that's only another symptom, isn't it?

that part should be fixable in and of itself, when injecting a IConfigProvider<TestSettings>

Yes, but currently we always inject a full settings provider.

the problem is that the SettingsViewModel itself needs the IGeneralConfigProvider ... it's just that TestExplorerViewModel is resolved before that so we don't see that issue yet...

Moreover, the TestExplorerViewModel currently needs a full settings provider, I think, because of the way it loads the settings dialog.

it loads the settings dialog?

It seems we need to clean the dependency-graph there either way...

There is not SettingsViewModel.

The settings window gets newed up every time as a SettingsForm taking the settings provider and operating system.

that's good, I guess?

Well, that means that whoever wants to open the settings has to have the correct instance of IOperatingSystem and a full settings provider.

hmm ... that sounds bad. I'll go do Security Homework first, though....

As long as there are views able to open the settings dialog, there will be a circle regarding the hotkeys. So, we will have to use property injection at some point to break the circle.

The property gets injected after objects have been created, so that everything works fine. We just have to find the correct place.

That problem is actually discussed in Dependency Injection in .NET using the example of MVVM in WPF.

[rubberduck-vba/Rubberduck] NelsonVides commented on issue #2497: Evaluate LibGit2Sharp v0.23/v0.24

blogs.msdn.microsoft.com/dotnet/2017/11/15/…

[rubberduck-vba/Rubberduck] Hosch250 commented on issue #2497: Evaluate LibGit2Sharp v0.23/v0.24

> Awesome. I don't remember at the moment, but I thought there was a deeper problem here than just the API breaking things. Maybe @rubberduck203 would remember?

[rubberduck-vba/Rubberduck] Vogel612 commented on issue #2497: Evaluate LibGit2Sharp v0.23/v0.24

> TBH I wouldn't even care much about destabilizing Source Control at this point ... It's marked experimental, 0.24 could even bring some stability into it. As long as it doesn't obviously break the build, we can still stabilize it as the issues crop up.

LOL:

-- This is as high as recursion can go if you still hit the max then there is nothing we can do.

source?

[rubberduck-vba/Rubberduck] Vogel612 commented on issue #3555: Set up a .settings file

> Follow-up discussion in chat

apparently I'm john snow ...

I really need to get that back into my damn head

preferrably without utterly deleting my confidence ....

@Vogel612 Work comment.

tehee

After all, recursion is to re-curse.

My boss keeps complimenting me on how efficient I am.

@ThunderFrame Does looking it up count?

@Hosch250 Interesting read. I think I pretty well understood the article.

Had time to look through my version of the WPF thing?

I downloaded the zip. I should be able to look at it for a few minutes before and after lunch.

Cool. I'd just have looked on GitHub if you couldn't clone it.

But you do whatever works for you :)

I don't realize while in VS how many extra files are created for a project.

TIL you can do select count (distinct item.Property).

soo ... github announced "Discussions"

github.com/blog/2471-introducing-team-discussions

seems like a viable tool to reduce meta and discussion issues IMO

So, are we calling off redesigning the declarations in favor of a refactor?

(Talking about discussions...)

seems like it

Cool.

either way it's a huge task that could whack every edge of RD a little....

One way or another, I wish we had an AST.

The CST is all fine and dandy, but every time you want to do something, you end up mucking around in all sorts of nodes you don't care about.

And if we had an AST, we could build it into the declarations.

Or build the declarations into it, or whatever.

On the other hand, our declarations are almost an AST.

Except for declarations alone.

Maybe I'll work on building an AST around the CST and we can decide how it looks.

@Hosch250 I'm caveman-primary-school-color-coding it right now. Articulation of questions will follow in a bit.

Thanks for this.

I would not say that the declarations are like an AST: they simplify constitute a symbol table.

Their structure is a little bit like one.

Simply because we have a tree of them that loosely maps the way they are structured in the code.

Perhaps a silly question - why would we want a tree that loosely maps the way it's structured in the code? Doesn't it make more sense to have a tree that actually maps to the structure to avoid omitting information that might be important for rewriting?

We don't use the declarations tree for rewriting.

We use the CST for that.

I just proved that calling .ToList() on an infinite data source eventually runs out of memory.

@Hosch250 I think I've fully groked.

Right, but if a declaration is only a loose mapping, that means you have to parse the CST to figure how to rewrite it?

I know it was either that or a stack overflow error.

No.

Well, actually yes for some nodes.

The declaration contains the CST node mapped to that symbol, though, so for most it's just a delete.

it's the "some" that worries me. :) That means you have more complicated code because some don't need to access, other needs to access the CST. I'd rather have one method to process the rewriting, even if it's just a deleting?

Well, we do.

or is that what you're aiming w/ AST?

Well, have you seen the rewriter code?

They all access the CST--we just say we want to delete a certain node, and it deletes it.

So, we can pass in a declaration, and by default, it just deletes the node.

RE: having seen it - Not enough of it, apparently.

On the other hand, we overrode it for certain nodes to do the more complicated digging. And yes, this actually did bring a bug into Move Closer to Usage that has yet to be fixed.

With the AST, deletion certainly could be handled better, if we built it to handle the whole CST.

That would be a huge undertaking, though, so basically I'm looking for something where we handle the main data structures just to make it easier to write inspections/refactorings.

Also, I found that calling .Count() on an infinite data source never finishes.

github.com/Hosch250/WpfApp2/blob/master/WpfApp2/App.xaml.cs#L12 sets the stage for all binding to be used and uses object initialization.

Yup.

Bear with me. I want to try and be precise as I can.

Did you notice how I removed that line from App.xaml so it wouldn't new up its own instance of the window?

Not yet. I didn't want to confuse myself going between yours and mine. I'm near certain what that is though.

Setting the DataContext allows TextBox to bind to the Text property of the view model in <TextBlock ... Text="{Binding Text, Mode=OneWayToSource, UpdateSourceTrigger=PropertyChanged}"/>

Yup.

The DataContext is where it looks for bindings.

Not sure about the order, but it also looks at parent windows for the bindings (does it look through all the parents to the root? Not sure.)

And it also looks in the code behind.

As I understand it. OneWayToSource is used because you want the source information to be updated as you type. I changed it to TwoWay but didn't see any feedback loop - that's what I was expecting. My assumption is that OneWayToSource has less overhead too.

Less overhead.

The feedback loop as you type is set by the PropertyChanged trigger.

Otherwise, it will update when the textbox loses focus.

Which has even less overhead, which is why it is the default.

When you type or edit the TextBox the way the other changes to the TextBlock displayed because of the UpdateSourceTrigger=PropertyChanged.

Well, in a way.

That value tells it to send the value to the VM right away.

Now, open the VM.

When you look at Text, notice how I call OnPropertyChanged on the property every time it is changed.

That is what tells the TextBlock to update.

This is picked up because ViewModel : INotifyPropertyChanged and each setter of the properties calls OnPropertyChanged which just invokes the PropertyChanged event.

@this The declarations build a tree in order to reflect the scoping of the declarations. For languages like C++ one would separate the scope from the symbols declared in it, but VBA only knows of scopes corresponding to declarations.

Yup.

If I only wanted the textblock to update on a button click, I'd remove that OnPropertyChanged call from the setter and do OnPropertyChanged("Text") in the button's Command.

I can barf gibberish onto the page but I'm trying to adequately describe, with correct terms, what's going on.

So, once we work through this, that can be something you work on.

But don't worry about it for now.

I understand the idea but lack the syntax to do it easily. I'd have to try and fail quite a bit before I arrived at the answer.

We can do that later. Just finish figuring this out.

The CallerMemberName supplies the propertyName to automagically be provided to it which tells the other properties that a change occurred. I'm not sure how better to describe it.

Basically.

IE Whatever binds to it that it may require a change to be done.

In WPF, everything listens for the PropertyChanged event on the VM.

So, if you want to force an update, you call it with the name of the property that has an update.

The CallerMemberName attribute automagically injects the name of the caller into the method.

Phone call. Sec.

Which is why we can get away with plain OnPropertyChanged() in a property setter--because the property name is passed in automagically.

@Hosch250 If not the name would have to be supplied to it IE Background and that doesn't bode well with refactoring.

Pretty much.

So, if you have:

public string Text
{
    set { OnPropertyChanged(); }
}

The compiler turns it into:

public string Text
{
    set { OnPropertyChanged(nameof(Text)); }
}

TextBlock has Text="{Binding Text, Mode=OneWay}". When the Text` property in VM is updated and the OnPropertyChanged event fires the text is updated with the newly updated information.

Yup.

And it knows to only listen to the property and not try to update it.

Mode=OneWay because less overhead and on account of it being a TextBlock you can't type into it.

Precisely.

Of course, I could set it up as a textbox with two-way binding, and it would both update and listen.

And if the other textbox was two-way too, they'd both always be in sync.

Foreground="{Binding Foreground}" binds the Foreground property of TextBlock to the public string Foreground property on VM. The string that's being returned is being consumed by a converter that automagically changes it from a string into a SolidColorBrush that the TextBlock.Foreground property is expecting.

Well, no.

There isn't a converter there.

I'm guessing behind the scenes, it's doing a string lookup on the name somehow.

I was actually a little surprised that it worked, but it's fine by me that it does.

Back in Windows 8 UWP, I actually did have to write a converter for that. Not sure the state of it now.

¿How does it work? I don't know and that's OK. I just know that it does work and for now that's all I need. I can research deeper if needs be.

Have fun doing that.

You'll get pretty deep into the guts of WPF before you find out :)

> ¿How does it work? I don't know and that's OK.

My comment was about the second half.

I don't need to dig that deep.

:wink:

Now, I didn't do it in this VM, but for most VMs to reduce overhead, you want to do something like this:

if (_text != value) { return; }

At the very top of the setter.

That way, it won't trigger an update unless it actually needs to.

^ I understood that one.

At first I was saying "Why do they have that guard clause??? :thinks: Ah.... It's the same value, no need to change."

ComboBox has ItemsSource="{Binding Colors}" which goes back to the DataContext` where it's set.

@Hosch250 just so I'm a bit clear -- why do we have VM in what should be a MVP pattern?

It can reduce overhead by a huge amount, and sometimes prevent infinite loops.

Why the heck would you use MVP in WPF???

Yup.

All bindings (in this app) go back to the VM, unless they are templates.

ComboBox's SelectedItem="{Binding Foreground}" AFAIK is acting like a setter for the Foreground` property. Again, anything else that's bound to it has changes reflected because of onPropertyChanged() at the end.

This app doesn't have templates, but that would bind to the specified properties on the type we were templating. So, if I had a list of Foo, I could write a template to use the Fizz and Buzz properties on Foo.

Same is true for the Background combobox as well.

But don't worry about that yet.

aha, I got that mixed up with window forms. We wanna MVVM. GTK.

@IvenBach Yep.

(incidentally i'm refactoring the dialog for extract method, and the code there has both VM and P. :\ )

@this Yeah, WinForms is MVP. I'll teach Iven that after he learns WPF.

@Hosch250 Pretty sure I understand that. 92.768repeat sure.

LOL.

I was going to chuck out the VM but now I see that it's the P I need to chuck out.

At least I got chicken.

Well, actually no.

it was too convoluted.

I mean, yeah.

That's what I understood from your example.

You'll still need a P to create the window in the first place :(

oh, the containing window form....

Yeah.

hmm, but it's supposed to be dumb

it's the WPF element that does everything, no?

It is. It should simply expose a .Show() method.

Yup.

It's not a dialog, so it doesn't need to expose an OnClosed event.

Very helpful seeing again the ObservableCollection<> to reinforce its usage. 'PropertyChanged` event and onPropertyChanged let me see how the properties were being updated, which led to me better understanding binding and how those changes are reflected.

WT* am I thinking?

It is a dialog.

yes it is. :)

@IvenBach An ObservableCollection is basically a List that fires OnPropertyChanged when you add/remove an element.

I could use a list here with no ill effects.

App.xaml.cs with DataContext = vm was what you were referring to about the <Application.Resources> earlier?

@Hosch250 But best practices is to use the ObservableCollection, right?

Could != Should

Not necessarily.

Pretty much, yeah.

@this github.com/rubberduck-vba/Rubberduck/blob/next/RetailCoder.VBE/…

@IvenBach Only if you need the UI to update when the collection updates, which is generally the case.

I think I thoroughly understood your example. That's mostly how it feels at least.

It doesn't matter what you use otherwise.

Just don't use a BindingList, please ;P

So, you are in VS, right?

ok, that makes much more sense, Hosch. Thanks for pointing me to that.

@Mat'sMug You might have another bookmark about binding now. chat.stackexchange.com/transcript/message/41260313#41260313

@Hosch250 I now know that one exists.

so in end, the P is to manage the opening/clsoing of the containing WinForm form, passing through the VM to the contained WPF element host control.

@Hosch250 Yes. I have VS open and that's how I was able to step through with callstack and see what was going on.

so, MVVM layered under the MVP

@this Yup.

Thanks!

OK, @IvenBach, create a new class called CommandBase

If my duckheadedness helps others at this pond learn I'm all for it.

Make it implement the ICommand interface.

For CanExecute, make it return true.

[rubberduck-vba/Rubberduck] rkapka commented on issue #3555: Set up a .settings file

    public abstract class CommandBase : ICommand
    {
        public virtual bool CanExecute(object parameter) => true;

        public abstract void Execute(object parameter);

        public event EventHandler CanExecuteChanged
        {
            add => CommandManager.RequerySuggested += value;
            remove => CommandManager.RequerySuggested -= value;
        }
    }

Actually, just paste that in.

And bookmark this article for further reading: msdn.microsoft.com/en-us/magazine/dd419663.aspx

(They call it the RelayCommand.

Add this constructor to it:

@Hosch250 Carpy/Posta for the win!

You can add this one too:

    public class DelegateCommand : CommandBase
    {
        private readonly Predicate<object> _canExecute;
        private readonly Action<object> _execute;

        public DelegateCommand(ILogger logger, Action<object> execute, Predicate<object> canExecute = null) : base(logger)
        {
            _canExecute = canExecute;
            _execute = execute;
        }

        protected override bool EvaluateCanExecute(object parameter)
        {
            return _canExecute == null || _canExecute.Invoke(parameter);

This is just overhead for making it really easy to implement custom commands.

Now, you need to define a DelegateCommand RefeshCommand { get; } command.

Some places like to do lazy command instantiation. Most WPF'ers will tell you to not bother.

So, you can add ` = new DelegateCommand(s => OnPropertyChanged(nameof(Text)));`

That's eager instantiation. I'll show you lazy instantiation later.

Now, add a button to the control and bind it's Command property to RefreshCommand.

@Hosch250 Working on this one. Trying to grok basic instead of blindly implementing.

Cool.

Oh, get rid of that ILogger object.

And the call to base.

The Action<object> is basically a void method that takes an object parameter.

The Predicate<object> is a bool method that takes an object parameter.

So, when you click your new button, it will call RefreshCommand.

There are other bindings you can use to set what value it passes as the argument you will see in s (CommandParameter).

If the command can only run with certain data, that's what CanExecute is for. The button will be disabled if you return false from CanExecute.

3 red squiglies.

Where.

'DelegateCommand' does not implement inherited abstract member 'CommandBase.Execute(object)'

Oh, duh.

I just copied that from RD.

'DelegateCommand.EvaluateCanExecute(object)': no suitable method found to override

Name OnExecute to Execute and EvaluateCanExecute to CanExecute.

'DelegateCommand.CanExecute(object)': cannot override inherited member 'CommandBase.CanExecute(object)' because it is not marked virtual, abstract, or override