RubberDuck

[MarkJohnstoneGitHub]

I've had a brief look at your project it looks great. I would highly recommend using RubberDuck https://rubberduckvba.com/ to help maintain your project. It's an extremely helpful VBA development tool.

Some observations.
I'm not a fan of Class Factory modules and probably can be implemented using a predeclared classes and a constructor method in the class. See VBA Attribute VB_PredeclaredId = True . I haven't used accessing classes outside of the current project with the VB_Exposed attribute I'm assuming it should work.

eg In a predeclared WebDriver class
Public Function Create() As WebDriver
Set Create = New WebDriver
End Function

A couple of good articles regard VBA OOP
https://rubberduckvba.wordpress.com/2018/04/24/factories-parameterized-object-initialization/
https://rubberduckvba.wordpress.com/2020/02/27/vba-classes-gateway-to-solid/

I'm a bit rusty on the VBA OOP possibly a Get Self property may be appropriate as demonstrated in the 2nd article. From memory you need to careful not using the default instance unintentionally.

IMO for class private variables it's clearer to place into a private type and declare a private variable named This. It's explained in one of those articles.

eg WebDriver class
Private Type TWebDrive
sessionId_ As String
driverUrl_ As String
'etc
End Type

Private This As TWebDriver

I'm using MS Access 2016 and WebShared.bas wouldn't compile due to Excel reference ThisWorkbook.Path . Is there a generic VBA method that would be appropriate? Maybe CurDir()? It however defaults to the user/documents instead of the application file location.
Other possible solutions see:
https://stackoverflow.com/questions/45854327/constant-that-indicates-access-or-excel

[GCuser99]

@MarkJohnstoneGitHub, thanks for the ideas/suggestions - will look into Rubberduck and the alternative Class creation method(s).

On the MS Access compile issue - is the ThisWorkbook reference the only problem you ran into to get this to work in MS Access, or is it just the first? If you have time/interest to look into what changes would be needed in total to make this compatible with MS Access, I certainly would be willing to consider implementing those changes, as long as we can preserve the current Excel functionality. I just don't have the time to do the brute-force trail blazing for that kind of out-of-scope endeavor - I bet there will be many more than that one issue, which by itself, seems very fixable.

If you are interested in making a contribution in that direction, would you be willing to open a new discussion on "MS Access Compatibility" and then use that to report on the problems/potential fixes encountered? That would be very useful and welcomed.

Thanks again!

[MarkJohnstoneGitHub]

In the Webshared.GetFullLocalPath the two ThisWorkbook references were the two compiliers issues I came accross. I'm not sure what the appropriate solution without proper investigation. I just temporarily replaced those with CurDir(). Note it returns the user/document path verses the workbook path, would that cause any functionality issues? Or the stackoverflow link might have a better alternative for a generic VBA solution. Maybe somewhere I've got similar code tucked away.

I tried blindly converting classes to preclared thou ran into issues, so now slowly progressing through it. Starting with the WebDriver and cleaning up code to help better understand the project. First I've got bit of learning curve to understand the Selenium WebDriver. There might be similarities to the webscrapping classes I did years back to dig up and compare. Out of the box your project should do what I require. The issues once starting to add other projects and things like the Factory classes start to become messy when dealing with 100's of classes.

There's probably a lot of questions like what error handling is there etc but will need some time to go through the code.

Once you start using RubberDuck code inspections you'll probably see a lot of recommendations.
eg In WebActionChain.SyncChannels
Dim lastKeyAction, lastMouseAction, lastScrollAction
'RD warning is implicitly Variant

A lot of variables declared ending in underscore. From memory that might cause issues when using Interfaces or could be never start or end module names with an underscore. Looks like most can changed by declaring a private type and This private variable which makes it clearer it's the classs variable and not a parameter or local variable etc. Typos could easily creep in causing headaches.

Some cleaning up is probably required, understandable with work in progresss. That's where RD would've come in handy to keep ontop of it instead of mass cleanup which might break code while cleaning up.

I'm a bit rusty with VBA after not touching it for a few years and was investigating some webscrapping with Selenium. Something was meaning to do years back when did some VBA webscrapping classes. I kept gettting sidelined and slightly disenchanted with due to the lack of decent VBA libraries to perform various tasks. You end up feeling you're reinventing the wheel in VBA.

I wrote a VBA Scripting.Dictionary wrapper as an alternative to Tim Halls version. I think TH version had major performance issues when dealing with large dictionaries. Also was an "exercise" in learning various VBA OOP techniques and hopefully a good example of any other methods learnt from the RubberDuck blogs. There might useful classes to check out. I did some advance SafeArray memory manipulation (headaches from diving into that lol) . The major advantage is that used Interfaces and can extend the Scripting.Dictionary easily eg a sorted dictionary or group by etc.

[GCuser99]

Wow - sounds like you have some work ahead of you.

Why not see if you can get it to work first (at the most basic level), and then deal with the code "cleanup"? You may end up spending a lot of time cleaning, only to find that there are several functional aspects that won't translate to MS Access without major refactoring. Seems like the code cleaning would be wasted effort in that case....

I just took a quick look at your Dictionary wrapper - very cool - thanks for sharing...

Like I said earlier, I will look into RD when I get time - it's not the highest priority since the code currently seems to work fine in Excel. But I do appreciate that it could be written better and may take that on (hopefully with the help of other contributors) when time permits. :-)

I do appreciate any progress that you can make in assessing the feasibility of using this in MS Access. Thanks.

[GCuser99]

PS: As a first pass feasibility assessment, I would use either of the ThisWorkbook.Path reference solutions that were presented as the accepted answer in the StackOverflow link that you mentioned in your original post - either one of those would allow you to move forward, and then once you are functional, we can return to that issue to find the best generalized solution for Excel/Access. I'm going to open a discussion on MS Access Compatibility that I hope you will contribute to. I suggest we leave coding style/cleanup for this RubberDuck discussion and not mix the two....

MS Access Compatibility

[6DiegoDiego9]

Hi @MarkJohnstoneGitHub, we put class constructors inside the SeleniumVBA addin module in order to be able to instantiate multiple objects of the same class, when keeping the SeleniumVBA addin module separate from the user code.
Yes, you can use the predeclared object from an external module but you would have no option other than using the class constructors to instantiate other objects of the same class.

RubberDuck: I already used it months ago to clean another project and it was very time consuming to spot the 5% good findings of the Code Inspector out of 95% junk or questionable findings, even after filtering only for Errors and Warnings (no Suggestions and Info).
This is why I still didn't do that in SeleniumVBA and postponed it to an indeterminate future time...

[MarkJohnstoneGitHub]

Gday @6DiegoDiego9 , yeah the Code Inspector spits out a lot of distracting warnings to wade through. A lot a good suggestions for writing better code and habits to get into. EG Instruction contains multiple declarations (clearer to dim one variable per statement). etc The implicit declarations are more alarming and confusing to be avoided, there are few of those.
The Code Explorer is also a useful tool when dealing with multiple projects by using the RubberDuck annotations you can group together classes into folder etc. Handy to quickly locate files related to a project and for exploring class properties and methods.

[6DiegoDiego9]

Hi @MarkJohnstoneGitHub, "Dim one variable per statement" (supposed it's exactly like this) is indeed one example of suggestion that I'd ignore most of times, especially in my private projects, as I often prefer to group related explicit declarations in a single instruction, like Dim foobar1 as string, foobar2 as string, foobar3 as string. I also think that proper grouping of declarations is good in public projects too.
Even more, in my private projects I often use the likely deprecated "$" for string and "&" for long declarations, like Dim foobar1$, foobar2$, foobar3$.

I'd very like to use Code Explorer too but with my big master library it was very slow to open and refresh. For example if I add a procedure, it requires a manual refresh hanging my VBE for seconds (7 to 35) in order to show it in Code Explorer. It also caused bad "random" hangs lasting seconds during my coding.
Also, I find confusing that the Code Explorer tree calls "function" a Function and "procedure" a Sub, while in Microsoft language Functions/Sub/Properties are all procedures.

I think that the slowness of Rubberduck and the inability (so far?) to fix it is the main reason why it's not stably used by VBA programmers.
In my case, I'm keeping it disabled and occasionally enable it only for specific short uses, waiting for it to mature...

[MarkJohnstoneGitHub]

@6DiegoDiego9 Rolls eyes lol. I've been doing a bit of that and refreshing my memory the pros and cons of such coding habits.
On that note the use of auto-instancing object variables (Dim and New object statement) did catch my attention, just couldn't remember offhand the cons . Best to avoid. See: http://www.cpearson.com/excel/DeclaringVariables.aspx

No good your having performance issuess with RD hopefully the next release might addrress those. It must be a vast project. I've examined fairly large ones the VBA.Net project, sadly it's only VBA 6 compatible. Would've loved a VBA 7 compatible reference they created to "hack" into the VBA enumeration. Also a lot of interesting design patterns to explore.

[6DiegoDiego9]

@MarkJohnstoneGitHub, the "As New" is another one that I use as a shortcut, when possible, despite general recommendations.
I'm also surprised that you recommend to avoid it while recommending to predeclare classes in SeleniumVBA because the predeclare attribute does exactly the same auto-instancing that you want to avoid. I mean, it doesn't pre-instantiate the class like one can believe. It just predeclares it, and instantiates it when first used at runtime exactly like the "As New".
So you can't test it for nothing too, if you need so.

[GCuser99]

I just used RubberDuck to add VB_Description attributes to all methods/properties in the WebActionChain class - very useful for that purpose indeed! As @6DiegoDiego9 mentioned, there were a lot of what seemed non-critical suggestions, some of which I implemented, and many that I deferred... Maybe the app would be useful in helping to enforce a coding-style standard, especially given the multiple contributors that the project enjoys. Something to consider...

[MarkJohnstoneGitHub]

WebKeyboard class could benefit to be predeclared and no need to instantiate and use the default instance. One of the rare times using the default instance might be appropriate. It appears to be a static class behaving similar to an Enum Type which can't store string literals. Other benefits might be some small performance gains.

EG. From the test_SendKeys
-no need for declaring or creating the WebKeyboard object
-I temporily created a predefined class WebKeyboardPD which test and worked fine. Maybe could add code so it cant be unintentional instantiated and enforce a Singleton class patern. I'll have to brush up on VBA Singleton classes.

keySeq = "Leonardo da VinJci" & WebKeyboardPD.LeftKey & WebKeyboardPD.LeftKey & WebKeyboardPD.LeftKey & WebKeyboardPD.DeleteKey & WebKeyboardPD.ReturnKey

[GCuser99]

@MarkJohnstoneGitHub - makes sense. What would be the downside if user "unintentionally" or even intentionally instantiated your WebKeyboardPD class?

The intentional case: I like the readability of keys.LeftKey over WebKeyboardPD.LeftKey, so I instantiate Dim keys as New WebKeyboardPD.

Would that cause a problem?

[6DiegoDiego9]

@GCuser99, I see no downsides other than wasting some microseconds at runtime for reinstantiating and reinitializating the class.

Anyway:

• if you're coding in the same project of the class you don't need to create another instance to "rename" it, just "Dim keys as WebKeyboardPD" and you're ready to use "keys"
• if SeleniumVBA is an external (referenced) project, you can't "Dim keys as New WebKeyboardPD" because the classes aren't externally "creatable" in VBA. You need to use the class constructor function

[MarkJohnstoneGitHub]

@GCuser99 It shouldn't cause any issues, excluding if was a enforced as true static/singleton class, i.e. by testing if it's the default instance in the Class_Initialize(). IMO would go against its intended purpose instantiating it.

"A static class is basically the same as a non-static class, but there is one difference: a static class cannot be instantiated. In other words, you cannot use the new operator to create a variable of the class type."

Regarding: 'Debug.Print keys.LeftKey 'this does not work : keys is NULL until being assigned/set an object.

If using the predeclared default instance you don't need the to Dim and create it etc.

After converting WebKeyboard to a predeclared class and removing the Dim and Set clauses from the test subs it worked as expect.

Sub test_sendkeys()
Dim driver As WebDriver
Dim keySeq As String

Set driver = New_WebDriver
driver.StartEdge
driver.OpenBrowser
driver.NavigateTo "https://www.wikipedia.org/"
driver.Wait 1000
keySeq = "Leonardo da VinJci" & WebKeyboard.LeftKey & WebKeyboard.LeftKey & WebKeyboard.LeftKey & WebKeyboard.DeleteKey & WebKeyboard.ReturnKey

driver.FindElement(by.ID, "searchInput").SendKeys keySeq
driver.Wait 4500

driver.ScrollToBottom
driver.Wait 4500
driver.CloseBrowser
driver.Shutdown

End Sub

[GCuser99]

@6DiegoDiego9, I could not get the "Dim keys as WebKeyboardPD" and you're ready to use "keys" to work. Here are my tests:

'Attribute VB_Name = "WebKeyboard"
'Attribute VB_GlobalNameSpace = False
'Attribute VB_Creatable = False
'Attribute VB_PredeclaredId = True
'Attribute VB_Exposed = True

Sub test_internally_referenced()
    Dim keys As WebKeyboard

    'Debug.Print keys.LeftKey 'this does not work
    
    Set keys = New WebKeyboard
    
    Debug.Print keys.LeftKey 'prints \ue012
    
    Debug.Print WebKeyboard.LeftKey 'prints \ue012
End Sub

Sub test_externally_referenced()
    Dim keys As WebKeyboard

    'Debug.Print keys.LeftKey 'this does not work
    
    Set keys = SeleniumVBA.New_WebKeyboard
    
    Debug.Print keys.LeftKey 'prints \ue012
    
    Debug.Print WebKeyboard.LeftKey 'prints \ue012
End Sub

Anyway, I'm not feeling the compelling reason to pre-declare, but I don't as yet see a downside either. It would seem to add another modality for doing things without affecting a user that either intentionally or unintentionally instantiates the class. So I'm indifferent - if this change makes some current and/or future users happy, then we should probably make the change(?).

[MarkJohnstoneGitHub]

IMO overall simplier to use, like an Enum Type and don't require to dim/set to use and no factory sub required. It more appropriately reflects WebKeyboard intended use as a static/singleton class. Technically not enforced as a singleton class until testing if it's the default instance in the Class_Initialize().
EG
'@description Creates a Singleton class which cannot be instantiated.
'------------------------------------------------------------'
Private Sub Class_Initialize()
If Not Me Is WebKeyboard Then
VBA.Err.Raise 429, "WebKeyboard ", "Cannot create object WebKeyboard ."
End If
'etc
End Sub

Yes will work unless enforced singleton class. Which leaves the door open to use either method so won't break existing code.

[6DiegoDiego9]

Hi @GCuser99, oh yes sorry, my example was not complete, here is the complete one :)
"if you're coding in the same project of the class you don't need to create another instance to "rename" it, just "Dim keys as WebKeyboardPD: Set keys = WebKeyboardPD" and you're ready to use "keys"

[6DiegoDiego9]

@MarkJohnstoneGitHub, factory sub is still required for the same reason it's required for a not predeclared class, that is because the classes aren't externally "creatable" in VBA. They can only be Private or PublicNotCreatable.
Even if you set the "Creatable" hidden attribute to True, it's ignored.

[6DiegoDiego9]

@GCuser99 my personal vote is to predeclare classes like WebKeyboard. I'd likely always prefer to use the predefined object for this class and I already had this in my personal (low priority) "todo".
About forcing it to be a singleton, I'd vote yes at the condition that the error clarifies: "Cannot create new object WebKeyboard. Use the predeclared instance instead".

[GCuser99]

I will push a commit this week to predeclare, along with some more RD method descriptions.

As for enforcing singleton... The reason I'm uncomfortable is because enforcement doesn't seem to protect the user from doing any self-harm, rather, at best it serves to educate the non-expert (like me!) about the advanced nuts and bolts of object instantiation, which I have personally enjoyed thank-you, but at worst will serve to confuse - how many users will know what a "predeclared instance" is? Seems like more potential downside than upside...

What about WebJsonConverter - should we predeclare that one too?

[6DiegoDiego9]

Good, thanks :)
Imho the singleton enforcing would serve at least to inform advanced users that the class is predeclared and so there's no need to instantiate it, since there's no native indicator in VBE that it's predeclared and RD Code Explorer (which indicates that) is likely used by <1% users.

Anyway I'm okay with not enforcing too, we can find other ways to inform everyone about that, for example by using the predeclared instance in tests and by leaving the Rubberduck keyword annotation ('@PredeclaredId) at the beginning of the module.

[MarkJohnstoneGitHub]

@GCuser99 It's VBA's OOP design pattern for implementing Static/Singleton classes. As mentioned earlier it's not a true static class as can still use a factory module to instantiate it. For all intended purposes it's treated as a static class, it's not a major isssue and was highlighting the design patttern for true static classes. Some things in VBA require to "tweak" for OOP functionality.

VBA Interfaces implement polymorphism and enforces it by being required to adhere to the to the interface.

On the note of Interfaces it's best to avoid using underscore in procedure names. It could cause issues.

There's a couple of properties in WebKeyboard Page_UpKey, Page_DownKey that should be renamed. If any other procedure names containing the underscore in the project they should also be renamed.

[GCuser99]

I like PageUpKey and PageDownKey better anyway! :-) Thanks.