Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WinGetPos Conversion- Consider Exception #100

Open
ronstudy1 opened this issue Apr 2, 2023 · 14 comments
Open

WinGetPos Conversion- Consider Exception #100

ronstudy1 opened this issue Apr 2, 2023 · 14 comments

Comments

@ronstudy1
Copy link

v1 code:
WinGetPos xx, yy, ww, hh, my_winTitle

actual v2 code:
WinGetPos(&xx, &yy, &ww, &hh, "my_winTitle")

expected v2 code:

try{
   WinGetPos(&xx, &yy, &ww, &hh, "my_winTitle")
}catch e{
   xx:=""
   yy:=""
   hh:=""
   ww:=""
}
@mmikeww
Copy link
Owner

mmikeww commented Apr 2, 2023

This would not be limited to WinGetPos, but nearly all of the Win* functions now throw this TargetError if the window is not found. I don't know that the converter should handle this. Is there a way to suppress the exception message such as with a #Directive ?

@ronstudy1
Copy link
Author

I am not aware of any directive or method to suppress this type of exception.
if the purpose of the converter is to transform scripts to v2 in a way that they behave identically to v1, then try {} ... catch {} should be used as demonstrated in the "expected v2 code" section.

@Lexikos
Copy link

Lexikos commented Jul 22, 2023

If the purpose of the converter is to transform scripts to v2 in a way that they behave identically to v1, the only practical approach, in my opinion, would be to provide a library of functions which behave as in v1.

Either way, you must take note of which errors would throw an exception and which errors would not. Many commands in v1 will throw an exception on failure if a try statement is active, but this is something that the converter cannot be expected to detect. It is better to let the exception be thrown, so that the user can learn how to deal with it, and so unexpected errors will be detected.

Unhandled exceptions can be selectively suppressed with OnError, but the only way to prevent TargetError from being thrown when there is a try statement active is to avoid calling the function. A function which wraps a Win function could selectively catch TargetError, but this may still fail to reproduce the exact v1 behaviour (see above).

@mmikeww
Copy link
Owner

mmikeww commented Jul 22, 2023

The purpose of the converter is not to have the output v2 script to have identical behavior as v1. IMO, the purpose is to ease the transition for the user to get onboarded to v2 as quickly as possible, and save them the work and hassle that is required for many of these tedious syntax changes.

My expectation was always that the user should review every line that was changed, cross check with the docs, and consider any new side effects that v2 might have introduced. Thats why in my original version I had the visual diff automatically launch after conversion, so that the user could inspect and see what required change.

@ronstudy1
Copy link
Author

user should review every line that was changed, cross check with the docs, and consider any new side effects that v2 might have introduced

This approach works well for those who haven't accumulated a lot of v1 code.
However, for those of us who are maintaining a zillion of v1 lines written over many years, with new code still being added on a daily basis, this approach is impractical. It may take several weeks of work, requiring a code freeze during the process.

Of course, even with "Identical conversion" there would still be a need to manually review and adjust the output, as 100% successful conversion is impossible.
Yet, if we have to manually go through each Win* call scattered all over the code, it's gonna be a real pain.


the only practical approach, in my opinion, would be to provide a library of functions which behave as in v1

This could be great.
It's possible to adopt such a library as an intermediate stage, just for achieving a super quick conversion.
After the user has a functional v2 code, they have the option to review the "library conversions" in their free time and perform a more professional and fine-tuned conversion using the "non-library convertor".

@mmikeww
Copy link
Owner

mmikeww commented Jul 23, 2023

This approach works well for those who haven't accumulated a lot of v1 code.
However, for those of us who are maintaining a zillion of v1 lines written over many years, with new code still being added on a daily basis, this approach is impractical. It may take several weeks of work, requiring a code freeze during the process.

I agree, but I'd ask, why even bother converting a script of that magnitude to v2? If it aint broke, dont fix it. In my opinion you should just stick with v1 (which is what I do for my very very large script). v2 breaks compatibility by definition so no matter what you do you will have to review if you want identical functionality. If you have accumulated lots of bug fixes over the years to get your code to perform how you want it with v1, you should not fool with it. Rewriting code just for the hell of it is usually a gigantic mistake. One of my favorite posts on the subject:

https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/

@ronstudy1
Copy link
Author

In my opinion you should just stick with v1

I don't know, maybe it's just a psychological issue, but it feels somewhat uncomfortable knowing that the majority of my extensively used scripts are based on a "deprecated" version.
Additionally, many of my large v1 scripts serve as libraries containing common functions. This fact prevents me from using v2, even in new scripts, as most scripts rely on those libraries..

At the moment, it doesn't seem too bad to continue using v1, as v1 and v2 mainly differ in syntax rather than capabilities. Furthermore, most of the forum threads still refer to v1. However, I'm uncertain about the long-term implications of sticking with v1.

Rewriting code just for the hell of it is usually a gigantic mistake. One of my favorite posts on the subject:
https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/

The current situation somehow differs from the Netscape example.
The conversion to v2 is not intended to improve code functionality but just to ensure the use of a supported language version.
Softwares that were originally written in old lanauges, such as Pascal or Ada, were eventaully needed to be converted to modern languages like C/C++.
A release of a non-backward compatible standard for C/C++ would lead to mutiny among software companies

@mmikeww
Copy link
Owner

mmikeww commented Jul 24, 2023

A lot of webhosts forced people to upgrade from PHP 5 to PHP 7, and that also caused dissent. But with PHP there were security issues. AHK might be more similar to the python v2/v3 upgrade, there was mutiny and some people still use v2. But AHK v1 has had years of bug fixes and stability improvements, I'd probably consider v1 the more stable version compared to v2 right now. Seems Lexikos is still releasing v1 builds that have bug fixes. He hasn't completely stopped support yet. I agree eventually it will happen, but even if/when it does, I still plan on using v1 even if its "unsupported". There shouldn't be a problem until Microsoft deprecates the API calls that AHK is based upon. Until then, I expect AHK v1 to work fine

Regardless, to ensure and guarantee identical functionality would probably require sifting through the AHK v1 and v2 C++ source code to check for all edge cases etc, and then writing intermediate functions like Lexikos suggested above. Building this converter is already enough of a daunting and tedious task, that I reckon it would be difficult to rally motivation for someone to take on that task. But if you have that motivation I will certainly encourage you and give you access to this repository to add your changes

@ronstudy1
Copy link
Author

There shouldn't be a problem until Microsoft deprecates the API calls that AHK is based upon

if and happens, hopefully AI engines would already become advanced enough to perform such conversions, requiring minimal manual adjustments if any

Building this converter is already enough of a daunting and tedious task

I understand and appreciate the time and effort required to build this converter, and I respect your decision not to consider exceptions in Win* functions if this is the final decision. Nevertheless, in the case of WinGetPos, we can achieve an identical conversion by:

try{
	WinGetPos(&xx, &yy, &ww, &hh, "my_winTitle")
}catch Error as err{
	if(err.Message=="Target window not found."){
		xx:=""
		yy:=""
		hh:=""
		ww:=""
	}else{
		throw err
	}
}

What are the reasons for not supporting it? Is it primarily due to the fact that it makes the code less readable, resulting in 13 lines instead of just one? Or are there other factors influencing this decision?

@mmikeww
Copy link
Owner

mmikeww commented Jul 24, 2023

What are the reasons for not supporting it? Is it primarily due to the fact that it makes the code less readable, resulting in 13 lines instead of just one? Or are there other factors influencing this decision?

Yes thats part of it for me, but maybe others would disagree about the code size. I think it looks a little sloppier and not as clean.

I'd also say that I think its inconsistent to add this change solely for WinGetPos, but then not for the other Win* funcs which also throw this TargetError. If we make this change, it gives the impression that the converter should be handling exceptions and then people might request the same in other cases.

Whereas, I think the fact that the exception exists in v2, allows the user to realize that they had a bug in their orig v1 script that they didnt know about. Perhaps they should've done a WinExist prior, and only do the WinGetPos call if the window already exists. The converter shouldn't be expected to fix bad code design. I'd be curious to know how/why your end script is relying on the output vars returning blank when the window isnt found

But these are just my opinions. If enough people think differently and are in favor of this, i suppose we can accept it

@ronstudy1
Copy link
Author

I'd be curious to know how/why your end script is relying on the output vars returning blank when

I can't recall the specific occurrence of WinGetPos in my code that triggered this entire thread. After reviewing all my WinGetPos instances, it seems that the code expects the window to exist, supporting your point that it might indicate a code bug

I think its inconsistent to add this change solely for WinGetPos, but then not for the other Win* funcs which also throw this TargetError. If we make this change, it gives the impression that the converter should be handling exceptions and then people might request the same in other cases.

I understand. If the converter's purpose isn't to achieve "identical conversion," then there's no need to address such thrown errors

I appreciate the time you spent to explain the considerations regarding both WinGetPos and the broader discussion about this converter's nature. Thank you.

@mmikeww
Copy link
Owner

mmikeww commented Jul 24, 2023

After reviewing all my WinGetPos instances, it seems that the code expects the window to exist, supporting your point that it might indicate a code bug

Well, maybe more "design bug" than "code bug". The exception forces you to think about how your code is supposed to perform. In most cases we would expect the window to exist, but if it doesn't, should we even be calling WinGetPos in the first place? Etc. Like, I think it should be something like:

if window exists
   wingetpos
   operate on win pos results
else
   do something else

rather than

wingetpos regardless
use pos results and compensate if results are blank

Its kinda hard for me to come up with an example off the top of my head. Thats why I was asking what your use case was, where you were running into the problem. Like suppose you wanted to move your AHK GUI to align to the right edge of a Notepad window:

wingetpos, notepad
winmove, AHK GUI hwnd,, notepad_x + notepad+w, notepad_y

This works fine if notepad exists, but if it doesn't, i guess it would move to 0,0 since those output vars would be blank? Not even sure how AHK would handle blank vars if it expects a number there. But then I mean if you are just compensating and youre ok with the GUI moving to 0,0 thats fine but I think thats a bug that could be handled better

@dmtr99
Copy link
Collaborator

dmtr99 commented Dec 7, 2023

I found a possible solution to the issue.

If you want the function to react more like the V1 function, please add the following code at the beginning.
(Credits to ntepa for this amazing piece of code, that shows how to acces a function that is overwritten.)

WinGetPos.DefineProp("Call", {Call:_WinGetPos})

_WinGetPos(this, &X?, &Y?, &W?, &H?, args*) {
    try {
        (Func.Prototype.Call)(this, &X, &Y, &W, &H, args*)
    } catch as Err {
      if (Err.Message = "Target window not found."){
         X := Y := W := H := ""
      } else {
         Throw ValueError(Err.Message,-1,Err.Extra)
      }
    }
}

But I would not change the behaviour of the convertor, it is in the nature of V2 to display more errors to get more trustworthy code.

@mmikeww
Copy link
Owner

mmikeww commented Dec 7, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants