RELOAD!

[rubberduck-vba/Rubberduck] 2 commits. 4 opened issues. 1 closed issue. 7 issue comments. 12038 additions.

[SimonForsberg/minesweeper-server] 1 commit.

[Zomis/minesweeper-flags-client] 21 commits. 220 additions. 78 deletions.

[Zomis/zomis.github.io] 1 commit. 1392 additions.

[Zomis/zomis.github.io-source] 2 commits. 3 additions. 2 deletions.

I want to fix the unhandled exception due to RCW disconnect on exits. Fortunately, we have CommandBase so I only need to implement it once. Unfortunately we kind of new commands all over the place and I have to add a new ctor injection basically everywhere. I considered just sticking in a static property but that would destroy the testability. I think the best way forward is to either change all those news into a factory call or just suck it up and pass a new ctor parameter along. Other ideas?

[rubberduck-vba/Rubberduck] comintern added label feature-todo-explorer to pull request #4827: TODO Explorer refresh

[rubberduck-vba/Rubberduck] comintern added label PR-Status: WIP to pull request #4827: TODO Explorer refresh

[rubberduck-vba/Rubberduck] comintern created pull request #4827: TODO Explorer refresh to merge comintern/Rubberduck/bugfix into rubberduck-vba/Rubberduck/next

> Refresh Todo Explorer to match other UI elements, refactor grouping to get rid of duplicated grid control. For those of us who have difficultly reading raw PNG data, it looks like this: !screenshot from 2019-02-25 18-06-15 Not much to it, just added the expand and collapse commands to match similar controls and changed the grouping to toggle buttons instead of a dropdown. Feel free to...

suggest different icons for the marker grouping and remove (or in the case above - "complete") icons. Sticky notes were a first thought, but I'm not sure if I'm sold on them. WIP pending some fixes to a couple other things that I found broken.

@this Some of the stuff we're newing up is specific to individual views - I'm not sure a factory is the best way to approach it generically.

I.e.:

    CollapseAllCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), ExecuteCollapseAll);
    ExpandAllCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), ExecuteExpandAll);

Yeah

The issue is that if we new it like that, then that class now has to pass around a IVbeEvents

Not for that it doesn't.

it would - i'm modifying the CommandBase

unless those doesn't derive from CommandBase?

No, they do. public class DelegateCommand : CommandBase

TBH I'm not sure they have any business knowing about IVbeEvents at all.

so they end up needing IVbeEvents for its ctor.

That's my point.

Only the ones that are in OLE wrappers.

ah, no

Split into 2 base classes?

if a WPF command touches a COM object then they need to be all hands off

we could do that but I feel that would be too error-prone?

That's my point - the 2 examples above don't come near COM.

hmm

They're bound to a grid, and expand and collapse the expanders for groups.

So, ComTouchyFeelyCommandBase?

ComAndGetItCommand

that means reviewing which each command has to be in the new derived base and thus take on IVbeEvents

LOL

In general, the ones that are currently injected are the "problem" ones.

Seems preferable than forcing all commands to take on IVbeEvents.

The ones newed up in the ctors are generally the UI specific ones.

Hmm.

I'm going to give that a try see, where I can get.

had a thought - if we do go down w/ a split class, those COM-touching commands need to go into their namespace.

That way, we can easily inspect for rogue access

That makes sense to me.

the wrinkle is that we have subfolders for Refactoring & MenuItems. Not sure I want to duplicate the subfolders.

Are you tracking down rogue EvaluateCanExecute calls?

well my plan is to override both the EvaluateCanExecute and OnExecute calls

and wrap with a check that the terminated event hasn't been called, and short-circuit otherwise.

OK, that's kind of what I thought. There still might be timing issues though.

yes, this wouldn't be a complete solution on its own

but a necessary component if we're going to avoid those rogue COM accesses at the shutdown

The context menus are particularly nasty in that regard.

^

[rubberduck-vba/Rubberduck] build for commit 0f527e5b on unknown branch: AppVeyor build succeeded

Oh wow, there's a ton of stuff that doesn't relocalize when the setting changes.

Invasion of the mole-bugs?

Nah, invasion of the "forgot about localization".

:lifts-rock: Look at what I found! Forgot about you a while back.

Oh damn... There's no way to change the localization of the inspection results without completely re-running them.

Do we want a complete reparse on language changes?

@Comintern Long-time issue.

That's another area my quick-fixes provider thing was an improvement--we can localize them, at least.

I'm not entirely sure it's that big a deal.

It's not like it's a really common thing to change back and forth.

Need to figure out the window captions too.

This is less "rabbit-hole" than "sarlacc-pit".

FML, there are timing issues too.

Home time.

How do we feel about putting code in Rubberduck.Resources? Specifically a static that will return all of the available localizations.

well, it is meant to be a "resource"

and resource file are basically backed by a static class sooo.

@Comintern FWIW, I say no. I mean, they ran it already and then decided to change the language after the fact?

No, it would simply return all the cultures that we have localizations for - that's it.

If I'm using a program that defaults to a gobble-gocky that I don't understand, the first thing I'm gonna to do is go to the settings/language, not muck around with refactorings or inspections, right?

Yeah, I think that's OK.

FWIW, it's not unprecedented, either - we have GUIDs and ProgIDs there.

I.e. public static List<CultureInfo> AvailableCultures

OK, that's where it's going to live.

you'd thunk Microsoft would have worked that out

IKR?

is there a reason to not make a command IDisposable?

Not having to track down everywhere you need to dispose it?

Obviously.

trying to decide if I should just not worry about unregistering the event

A .NET event?

VbeEvents is just a .NET event forwarder

so if I don't unregister it, no harm should be done

in theory

Force clear the delegate collection from the dispose on VbeEvents.

Don't make the subscribers unregister themselves.

hmm good idea. I like that.

IIR I did that with the sub-classes because of timing problems.

I think i got the title for the PR

"Install the new timing belt for Rubberduck"

lol

Rubberduck has 100K miles on it?

just to confirm that's what you meant by clearing the list:

EventsTerminated?.Invoke(this, EventArgs.Empty);
EventsTerminated = null;

I would assume it already long ago passed 1M mark.

I think it might be better to explicitly clear the collection.

Let me try to track down an example. I can't remember where exactly I did that.

SubclassingWindow.ReleaseHandle()

private void ReleaseHandle()
{
    lock (_subclassLock)
    {
        if (!_listening)
        {
            return;
        }
        Debug.WriteLine("SubclassingWindow.ReleaseHandle called for hWnd " + Hwnd);
        var result = RemoveWindowSubclass(Hwnd, _wndProc, _subclassId);
        if (result != 1)
        {
            throw new Exception("RemoveWindowSubclass Failed");
        }
        ReleasingHandle?.Invoke(this, null);
        ReleasingHandle = delegate { };
        _listening = false;

yeah, i just saw that

going to do that instead.

FWIW - the first SO hit says null is just fine. :-\

Yeah, I figured the empty collection was safer.

setting to null actually sounds more idiomatic, come to think of it

> A null reference is the canonical way of representing an empty invocation list, effectively.

@this ^ I read that as more than "just fine" :)

Jon Skeet makes it fine.

Yeah

I don't blame Comintern, though - see how much trouble I caused with that null for that IEnumerable<AutoHostMacro>....

:)

Yeah, I may be a lot of things, but I'm not Jon Skeet.

@this just asking could we make null a new Rubberduck type so we know which kind of null irrespective of Access or SQL or Excel emptycell null and handle this better.

in C# that would have to be @null

... not really? Note that the null also have different semantic meanings along the contexts/languages.

oh, some Rubberduck.IsNullOrEmpty(ByVal value As Variant) As Boolean?

the word "null" suffers from excessive overloading if you ask me.

@MathieuGuindon but why need RD? That can be just a plain VBA function

true

better as such, too

...knowing the implementation of null differs

(FWIW, we have one like that - IsNoData to cover all cases)

don't shoot me over the lame name!

Public Enum Nils
    [Nothing]
    [Null]
    [Empty]
End Enum

[DBNull]

Heh. Can't forget the nbsp.

Public Enum Nils
    [Nothing]
    [Null]
    [Empty]
    [DBNull]
    []
End Enum

Public Function IsNoData(InputData As Variant) As Boolean
    Select Case True
        Case IsObject(InputData)
            IsNoData = (InputData Is Nothing)
        Case IsArray(InputData)
            Dim l As Long, u As Long
            On Error Resume Next
            l = LBound(InputData)
            u = UBound(InputData)
            If Err.Number Then
                l = 0
                u = 0
            End If
            On Error GoTo 0
            IsNoData = ((l = 0) And (u = 0))
        Case Else

The IsArray case has a false positive:

Sub foo()
    Dim bar(0)
    Debug.Print LBound(bar), UBound(bar)
End Sub

is that really an "empty" array?

depends on what we mean by it, I guess

to me, bar(0) isn't empty, even if it contains no value ATM.

Nope. It's an array with a single element.

^

oh, wait - that should be -1 for u

That presumes that the lbound is 0.

Dim bar(-1 To -1)

Gotta love them SAFEARRAYs

I'd love to ask MSFT why they thought we did really need to get RTEs with *Bound functions.

the problem is that with either function, you get a RTE

I guess that should be the only test

I have a function for that.

let's see.

I think it should be:

    Case IsArray(InputData)
        Dim l As Long, u As Long
        On Error Resume Next
        l = LBound(InputData)
        u = UBound(InputData)
        IsNoData = (Err.Number)
        On Error GoTo 0

:+1: That is more thorough. I know that the function above was written to avoid using APIs originally hence the loosey-goosey checking abusing VBA's voodoo conversions.

It's probably not production ready, no.

> [System.Reflection.AssemblyCopyrightAttribute("Copyright © 2018")]

^ That wants a script.

hmm, i think that should be covered in the base project?

failing that, yes, a script.

I'm not entirely sure how that works.

actually I don't see it in the base project

oh youre kidding me

github.com/rubberduck-vba/Rubberduck/blob/…

That does not belong there

:facepalm: it's all over various csprojs.

nor should that exist: github.com/rubberduck-vba/Rubberduck/blob/…

Yep, we rock at copyrighting.

Most OSS projects are better at copylefting.

I'm inclinced to just fix them all up and have it centrally managed by the base project

not sure what an attribute buys us - if the csproj can do it, it'd be more simpler to manage it from the base csrpoj file.

Agreed.

ok, i need a reality check. I'm updating the mocked code explorer & associated tests

those now are ComCommandBase-derived, so they need a Mock<VbeEvents> passed into it.

The thing is that several tests either 1) new them directly or 2) use reflection to new it up.

I could cheat and just pass in null, since none of those tests rely on the VbeEvents but I was worried that if I do this, then it makes any future modifications harder.

You mean the ImplementFooCommands?

that's one of things to change, yes

Isn't there a VbeBuilder overload that you can get the events from?

but also consider the AddsTestModule as an example (in UnitTestCommandTests)

that's what I'm looking at ATM

        [Category("Commands")]
        [Test]
        public void AddsTestModule()
        {
            var (vbe, state) = ArrangeAndParseTestCode(ComponentType.StandardModule, "Module1", string.Empty);
            using (state)
            {
                var addTestModuleCommand = new AddTestModuleCommand(vbe.Object, state, ArrangeCodeGenerator(vbe.Object, state));

                addTestModuleCommand.Execute(null);

                var expectedModule = $"{TestModuleBaseName}1";
                var generated = state.DeclarationFinder.AllUserDeclarations.SingleOrDefault(test => test.Id…

Return the event sink from ArrangeAndParseTestCode?

that's what I was going to do but that doesn't address the original problem: if you change the ctor, you gonna bonk a lot of tests

because it's then new'd up after the ArrangeAndParseTestCode

I almost want to say that ArrangeAndParseTestCode ought to return a command.

instead of (vbe, state)

maybe just (command, state)

So ArrangeAndParseTestCode<T>?

(with a different name)

For those tests, passing a null should be fine though, right?

Correct

those don't rely in any way on the vbeEvents

If you're adding tests for EvaluateCanExecute though...

I think there were a bunch of those in the CE tests IIR.

it's more that I want to avoid fragile tests that goes belly up just because you added a new ctor parameter.

Oh, then you should have seen it before I got to it.

ideally, there should be only one place.

^

Yeah that's why i'm trying to decide if I should cram it into the ArrangeAndParseTestCode

and have it return the (command, state) instead of newing it up in the test

TBH, that could probably use some more coverage.

doesn't everything?

I went a bit light on the VM tests because of the coverage in the model and engine.

Testing pyramid and all.

TODO: Construct the Pyramid of Giza

Needs more rocks. Preferably square ones.

actually it would be more trapezoids. at least for those on the outside edge.

thinking about it some more, I think my best approach is a new function GetCommand<T>(vbe, state) with vbeEvents as an optional parameter

Should I display the localized language name in parens there?

I.e., if it's en-US, show "Deutsch (German)" or something like that?

Probably overkill now that I think of it. Why would you pick "Deutsch" if you didn't know it was "German" in en-US?

[rubberduck-vba/Rubberduck] comintern assigned comintern to issue #4818: Settings form fails to load with language codes that aren't implemented.

well, do we install in localized language?

I don't remember (don't think, either) that installer sets the localization for RD automatically

so that would be a bigger problem for one who doesn't know enough English. OTOH, that same person is screwed because they have to mouse around to find the language.

they would have found the repo

hmm good point.

that'd require a minimum comprehension in english at least.

why does the AddTestModuleWithStubsCommand take AddTestModuleCommand as a ctor parameter?

that kind of feels wrong.

it's forcing me to temporally couple the mocks. :(

Well right now, I have it showing the CultureInfo.NativeName.

Always.

I think that's reasonable, TBH.

If I speak Martian, I'm gonna to instinctively go for ∞†©©∞˙˙ßœ instead of "Martian", right?

I was actually considering ditching the Language_* resources entirely - I think they're already localized by the underlying CultureInfo.

^^

The ∞ is silent, BTW.

I am good with ditching the resource. I figured they were a hack around for the language thing anyway.

I'm looking at the AddTestModuleWithStubsCommand and it makes less sense.

:-D

git blame me

AFAICT, all it does is just invoke the AddTestModuleCommand

Yep.

I can see it if it was an unfinished thought but it really really wants to derive from AddTestModuleCommand, not take it as a ctor arg. Argee?

and of course add a TODO: This class is insane. Make it sane again

Probably. It's calling an overload on ITestCodeGenerator.AddTestModuleToProject

Oh wait. I think I remember why it's messed up like that.

There are 2 of them, so the inheritance was messed up.

Look in Rubberduck.UI.CodeExplorer.Commands.

:facepalm:

^ Has zero clue what they're talking about.

Maybe I should just chuck that AddTestModuleWithStubsCommand altogether

Wut?

I hand carved that out of a pile of random code.

Good for you. you can mount it on the wall if you want.

:D

I'll put it on top of my drawings of localizations.

Seriously, if it doesn't anything that's in fact different from the AddTestModuleCommand, it's probably better out of the codebase until someone implements one that actually does add stubs

because otherwise it's just adding noises to the unit tests

No, it adds stubs.

Seriously. There's an overload of ITestCodeGenerator.AddTestModuleToProject that handles the code generation.

That's why AddTestModuleWithStubsCommand passes the argument for the target module.

Ok, that was not clear from reading the code.

Yeah, I probably could have gone a bit further with the refactoring on that one.

I would hve expected the ITestCodeGenerator to be passed in and configured accordingly

I saw diamond pattern and said "F it."

Gotcha

sorry for not catching your dilemma earlier

Meh, it was the person before me's dilemma more than anything.

this does make the unit tests ugly because the mocks are then temporally coupled

That should be OK functionally though. It just moves the AddTestModuleWithStubsCommand test up a row in the pyramid.

BTW, I'm not attached to the current architecture by any stretch of the imagination.

I mainly let sleeping tech-debt lie.

yeah, and this is a one big tech-debt too

ATM, it's:

        var (vbe, state) = ArrangeAndParseTestCode(ComponentType.StandardModule, "Module1", string.Empty);
        using (state)
        {
            var addTestModuleCommand = ArrangeAddTestModuleCommand(vbe, state);
            var addWithStubsCommand = ArrangeAddTestModuleWithStubsCommand(vbe, addTestModuleCommand);

the problem, though is that all 3 Arrange* there will create their own mocked VbeEvents. Not good

will need to rethink that

There's not really a reason that the commands need to be created before the parse though.

I think I did it mainly for convenience.

isn't that becuase you have to pass state in?

You can get the state before you actually parse it. The TestEngine tests do that IIR.

ok

Was there an open issue for an NRE in CodeMetricsViewModel?

yeah faceplant

github.com/rubberduck-vba/Rubberduck/issues/4814

this is actually an issue + a suggestion rolled in one, though

suggestion being that nothing should be bonking the parser just because some external subsystem tripped over its own feet.

OK, that's a completely different one.

var results = _resultsByDeclaration?.FirstOrDefault(f => ReferenceEquals(f.Key, SelectedItem.Declaration)); faceplants when there isn't a selected item.

Any other locales with cropping issues in the Settings window?

German? ;-)

wait, is there a GridSplitter there?

If there is, I can't find it.

would it be good UX to allow resizing the side panel?

I'm inclined to say no.

I'd much rather let it take twice the line height than have to horizontally scroll

but that's just me (I have a thing about horizontal scrolling)

wrap content isn't crazy at all

TBH, the window could stand a re-design.

^

how would it be done better?

The panel layout is quite common & intuitive

Good question.

The main issue we're running into with the panel layout is that the panels don't share similar structures.

Some of them are really crowded too.

hmm. I thought the whole purpose of the panel layout was to allow discrete structure to be presented from a single dialog.

I do agree w/ the crowding, though - that is a problem esp. w/ the hotkey settings or inspection settings for example.

Right, but the amount of real estate each one takes is different.

just so we're clear - why can't hotkeys have its own space?

Panel layouts also support plug-ins much, much better than alternatives.

@this they should

Inertia?

^

Hotkeys is the easy one

Code inspection, OTOH.

do we put Misc + whitelist identifiers in their own panel?

So what happens when you change the settings mid-parse?

I'd hope it has no effect since parser has its state and won't read until it finishes....

The operative keyword being "hope"

Inspection settings?

come to think of it -- Whitelist identifier is just specifically for one inspection....

Like in the middle of an inspection? Is it just hit or miss as to whether it "takes"?

That is a good question. I honestly don't know.

ideally, the inspector should have a copy of the settings before it start running inspections.

I wonder if the whitelist identifier should be actually a subgrid under the meaningful name inspection....

makes it much less discoverable, though....

I'm almost thinking that saving the settings should cancel the parse.

That might be disruptive though.

actually that would be simpler

we don't immediately write to the settings when leaving the panel (without closing dialog), do we?

The OK button immediately writes it, and the dialog is modal.

yes but I'm talking about changing the panel

Oh, not per panel.

It's all or nothing ATM.

ok, that's good, I think.

But you could conceivably change settings n times in the middle of a parsing run.

could wire the Ok button up so that if it see that parser is not in Ready state, to request a new parsing

That's kind of what I was thinking.

Actually, it does write "per panel" technically - only the dirty ones get a write call IIR.

@this Did you ever find the repro code for #4814?

I suspect an Enum or Type. Probably an Enum.

let me try that again.

You saw my comment, right?

yes

woah that's messed up

Yep. That's a Declaration equity check error all right.

that does parse, though

Same issue as #4714.

QMN doesn't handle those "gracefully".

Interesting...

hmm. there is one more difference

in my original report, I was seeing parsing on every keystroke i made into the properties toolwindow

Excel has issues loading super long file names.

in a new file I cannot repro this

Huh. I'm not sure what would cause that.

me neither. I can't repro this either.

but it definitely was happening. As mentioned it was during a lengthy session where I was moving around a lot fo stuff

VBE bug?

isn't VBE a bug?

Bugs can have bugs?

Like lice on a cockroach.

I supose so.

[rubberduck-vba/Rubberduck] comintern created pull request #4828: Misc bug fixes. to merge comintern/Rubberduck/next into rubberduck-vba/Rubberduck/next

[rubberduck-vba/Rubberduck] build for commit 4d588f63 on unknown branch: AppVeyor build succeeded

[rubberduck-vba/Rubberduck] bclothier merged pull request #4828: Misc bug fixes. from comintern/Rubberduck/next into rubberduck-vba/Rubberduck/next

[rubberduck-vba/Rubberduck] comintern pushed commit d4d97a89 to next: Return empty enumerable instead of null. Closes #4824

[rubberduck-vba/Rubberduck] comintern pushed commit ac134834 to next: Fix NRE when no item is selected.

[rubberduck-vba/Rubberduck] comintern pushed commit 4d588f63 to next: Add validation to locale setting, remove hard-coded resource options. Closes #4818

[rubberduck-vba/Rubberduck] web-flow pushed commit 50a071f8 to next

[rubberduck-vba/Rubberduck] bclothier closed issue #4818: Settings form fails to load with language codes that aren't implemented.

[rubberduck-vba/Rubberduck] bclothier closed issue #4824: Refactor|Rename dialog box will not display

[rubberduck-vba/Rubberduck] rubberduck-vba-releasebot published prerelease Prerelease-v2.4.0.4573

[rubberduck-vba/Rubberduck] rubberduck-vba-releasebot created tag Prerelease-v2.4.0.4573

[rubberduck-vba/Rubberduck] build for commit 50a071f8 on next: AppVeyor build succeeded

So if we have bugs begetting bugs, what does this leaves us eh @this? I thought Rubberduckers are becomeing exterminators? @Comintern like Daleks!

[rubberduck-vba/Rubberduck] Greedquest forked us into Greedquest/Rubberduck

[rubberduck-vba/Rubberduck] Greedquest created pull request #4829: Minor corrections to bug_report.md to merge Greedquest/Rubberduck/patch-1 into rubberduck-vba/Rubberduck/next

> Fix typos and capitalise for consistency

[rubberduck-vba/Rubberduck] paulbaumgartner created comment on issue #4823: Rubberduck refactor menu not active (running on mac via parallels)

[rubberduck-vba/Rubberduck] paulbaumgartner edited issue #4823: Refactor menu not active (running on mac via parallels)

[rubberduck-vba/Rubberduck] MDoerner created comment on issue #4823: Refactor menu not active (running on mac via parallels)

[rubberduck-vba/Rubberduck] paulbaumgartner created comment on issue #4823: Refactor menu not active (running on mac via parallels)

[rubberduck-vba/Rubberduck] comintern commented on .github/ISSUE_TEMPLATE/bug_report.md of pull request #4829: Minor corrections to bug_report.md

@Duga @Comintern I can confirm that it fixed #4824

@FreeMan Woot!

[rubberduck-vba/Rubberduck] bclothier created comment on issue #4823: Refactor menu not active (running on mac via parallels)

And I feel human again. D0ub13 w00t!!

@FreeMan Lesson: don't pull from this' work. Pull from comintern's work instead. :D

makes a note

@Duga I looked at the wiki but I'm a bit fuzzy whether editing inside procedure will still require refreshing the referenced modules. I would hope no but ? - 2 minutes on a reparse is kind of excessive.