iseahound / ImagePut

A core library for images in AutoHotkey. Supports AutoHotkey v1 and v2.
https://www.autohotkey.com/boards/viewtopic.php?f=83&t=76633
MIT License
116 stars 24 forks source link

Recursion error on AHK_H2 #30

Closed anonymous1184 closed 10 months ago

anonymous1184 commented 10 months ago

Hello!

First of all, terrific job! While I don't use your lib, quite often recommend it to other users (usually the less experienced). It is one of the few one-fits-all that actually works; plus it is very easy to get it working. Thanks for the superb effort.

So... a user raised an issue in the AHK_H2 repo about a recursion problem, the lib has a "sentinel value", that conflicts with an H-only feature:

; H adds this:
; Object.Prototype.Get()

; This creates a recursion problem:
keywords := {base: {__get: this.get}}

There are two ways of addressing the issue, renaming the method (say this.got):

keywords := {base: {__get: this.got}}

Or explicitly pass the object reference rather than rely on this:

keywords := {base: {__get: (self, name, *) => this.get(self, name)}}

That in turn will require the method to change:

; From:
static get(name, p*) {
   return ObjHasOwnProp(this, name) ? this.name : ""
}

; To:
static got(name, p*) {
   return ObjHasOwnProp(self, name) ? self.%name% : ""
}

In any case, the method has a bug. It will always try to grab the .name property instead of evaluating the name variable into a dynamic property.

Let me know if you are interested in adding support for the H release to create a PR.

iseahound commented 10 months ago

The sentinels "get" have long been planned to be removed and updated to use ? (maybe / unset) instead. This would however force the library to be on the v2.1 alpha releases, would that be fine?

anonymous1184 commented 10 months ago

Your call, but one of the most alluring aspects of the library is how easy if to just plug & play; asking users for an alpha will put a dent on the usability.

If you want, I can remove the sentinel and validate the input with IsSet()/.Has()/.HasOwnProp() (depending on what's needed), that way the compatibility increases instead of getting reduced.

iseahound commented 10 months ago

The sentinel was designed to reduce my workload of maintaining scripts for v1 and v2 by mimicking v1's property of making everything the empty string. Either of the above suggestions is fine (using self or got) and I'll be happy to merge.

anonymous1184 commented 10 months ago

PR: https://github.com/iseahound/ImagePut/pull/31. Tests passed.