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

Basic implementation of SysLink controls #9

Merged
merged 12 commits into from Feb 1, 2012

Conversation

3 participants
@ChrisS85
Contributor

ChrisS85 commented Jan 11, 2012

It's probably missing some styles and maybe other features, but adding them works ok and g-labels also work.

In script the URL text isn't available yet; I didn't know how to do that since in GuiWindowProc gui_event and event_info were defined as numbers instead of variant types or strings.

It would be nice if you could look over it and provide me some pointers where I need to add/change code.

ChrisS85 added some commits Jan 11, 2012

Basic implementation of SysLink controls.
It's probably missing some styles and other features, but adding them seems to work ok.
Added AltSubmit property for Link controls:
If not used the control will try to execute the URL of the clicked link.
If used, it is handled through its glabel.
@maul-esel

This comment has been minimized.

Show comment
Hide comment
@maul-esel

maul-esel Jan 19, 2012

Any news on this? It looks like a great feature to have.

maul-esel commented Jan 19, 2012

Any news on this? It looks like a great feature to have.

@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 19, 2012

Contributor

Not from me, I haven't heard anything from Lexikos.

Contributor

ChrisS85 commented Jan 19, 2012

Not from me, I haven't heard anything from Lexikos.

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 20, 2012

Owner

Good work.

If a user defines a g-label, they presumably want it to be called at some point. I think that requiring the AltSubmit option to enable the g-label is unintuitive and inconvenient. The simplest alternative is to simply check for control.jump_to_label instead of AltSubmit. However, since a link control can contain multiple links and each one can have a different href attribute (or none at all), I suggest the following behaviour:

  • If the item has a URL, attempt to execute it with PerformAction().
  • If PerformAction() failed or wasn't attempted, call the control's g-label.

My initial thought for AltSubmit was to have it bypass PerformAction(). However, if you don't want a URL to be executed, you can simply omit the href attribute. On the other hand, if the URL was readily available, the g-label could launch it via an alternative method (or in an alternative browser). Since the URL isn't readily available, perhaps AltSubmit should be reserved for a more useful purpose.

It is worth noting that although MSDN says "An HREF can be any protocol, such as http, ftp, and mailto", it doesn't appear to do any validation, so something like <a href="calc">Run Calculator</a> or any other command supported by PerformAction() will also work.

Lastly, there are build errors in the ANSI (mbcs) build configurations:

  • item.szUrl is Unicode only. CStringTCharFromWCharIfNeeded provides one workaround.
  • WC_LINK is Unicode only. I'd suggest simply replacing it with _T("SysLink").
Owner

Lexikos commented Jan 20, 2012

Good work.

If a user defines a g-label, they presumably want it to be called at some point. I think that requiring the AltSubmit option to enable the g-label is unintuitive and inconvenient. The simplest alternative is to simply check for control.jump_to_label instead of AltSubmit. However, since a link control can contain multiple links and each one can have a different href attribute (or none at all), I suggest the following behaviour:

  • If the item has a URL, attempt to execute it with PerformAction().
  • If PerformAction() failed or wasn't attempted, call the control's g-label.

My initial thought for AltSubmit was to have it bypass PerformAction(). However, if you don't want a URL to be executed, you can simply omit the href attribute. On the other hand, if the URL was readily available, the g-label could launch it via an alternative method (or in an alternative browser). Since the URL isn't readily available, perhaps AltSubmit should be reserved for a more useful purpose.

It is worth noting that although MSDN says "An HREF can be any protocol, such as http, ftp, and mailto", it doesn't appear to do any validation, so something like <a href="calc">Run Calculator</a> or any other command supported by PerformAction() will also work.

Lastly, there are build errors in the ANSI (mbcs) build configurations:

  • item.szUrl is Unicode only. CStringTCharFromWCharIfNeeded provides one workaround.
  • WC_LINK is Unicode only. I'd suggest simply replacing it with _T("SysLink").
@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 20, 2012

Contributor

Do you know how to get the URL in one of the event variables so that it can be accessed in script?

I wasn't sure how to best check for a valid URL (Regex or some windows API?) which is why I kept separate behvaior with AltSubmit option. But it can be done better by checking for the existence of the g-label like you mentioned.

Contributor

ChrisS85 commented Jan 20, 2012

Do you know how to get the URL in one of the event variables so that it can be accessed in script?

I wasn't sure how to best check for a valid URL (Regex or some windows API?) which is why I kept separate behvaior with AltSubmit option. But it can be done better by checking for the existence of the g-label like you mentioned.

ChrisS85 added some commits Jan 20, 2012

-Fixed build errors on Ascii build
-SysLink control will now try to execute the links when no g-label is specified and execute the g-label otherwise.
@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 20, 2012

Contributor

I implemented your suggested changes.

Link URL can be retrieved later by LM_GETITEM. Can this be integrated in the event handling function so it is visible as event variable or would it be better to implement it in GuiControlGet?

Contributor

ChrisS85 commented Jan 20, 2012

I implemented your suggested changes.

Link URL can be retrieved later by LM_GETITEM. Can this be integrated in the event handling function so it is visible as event variable or would it be better to implement it in GuiControlGet?

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 20, 2012

That still doesn't build. CStringTCharFromWChar has an implicit conversion to LPCTSTR, but not LPTSTR, which is what ActionExec accepts. An explicit cast is required to remove the const modifier. It compiles in Unicode configurations because the macro just returns its input unchanged (hence "IfNeeded").

Generally it is preferred to limit each commit to one distinct set of changes. (I guess you'd call it an "atomic commit".) If you must commit multiple changes, the first line of the message should reflect it since that's often all that is visible. In this case I was expecting only fixes for build errors, but there was also a change to behaviour.

Lexikos commented on d72affa Jan 20, 2012

That still doesn't build. CStringTCharFromWChar has an implicit conversion to LPCTSTR, but not LPTSTR, which is what ActionExec accepts. An explicit cast is required to remove the const modifier. It compiles in Unicode configurations because the macro just returns its input unchanged (hence "IfNeeded").

Generally it is preferred to limit each commit to one distinct set of changes. (I guess you'd call it an "atomic commit".) If you must commit multiple changes, the first line of the message should reflect it since that's often all that is visible. In this case I was expecting only fixes for build errors, but there was also a change to behaviour.

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 20, 2012

Owner

I did not test it because I didn't know what values of the build configuration needed to be changed for ASCII (charset->multibyte?).

You're right about commits but it's easy to forget about it while working on the code.

Owner

ChrisS85 replied Jan 20, 2012

I did not test it because I didn't know what values of the build configuration needed to be changed for ASCII (charset->multibyte?).

You're right about commits but it's easy to forget about it while working on the code.

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 21, 2012

If you're using the included vcxproj file, no changes are needed. Just switch to any of the "(mbcs)" build configurations. If you're not using it, I guess you're using an older version of VC++?

Lexikos replied Jan 21, 2012

If you're using the included vcxproj file, no changes are needed. Just switch to any of the "(mbcs)" build configurations. If you're not using it, I guess you're using an older version of VC++?

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 22, 2012

Owner

I didn't try that config because I wasn't sure what it meant by its name.

Owner

ChrisS85 replied Jan 22, 2012

I didn't try that config because I wasn't sure what it meant by its name.

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 20, 2012

When checking for an empty string, the fastest way is to check just the first character. If it is a null character (zero), the string is empty; otherwise the string is not empty. Also, I presume you meant to check szUrl rather than szID.

if (*item.szUrl)  // URL is not empty.

What should happen if the URL cannot be executed? Currently I think nothing happens. Perhaps the g-label should be called so that fallback code can be written. For instance,

if (!*item.szUrl ||
    !g_script.ActionExec((LPTSTR)CStringTCharFromWCharIfNeeded(item.szUrl).GetString(), NULL, NULL, false))
    pgui->Event(control_index, nmhdr.code, GUI_EVENT_NORMAL, item.iLink + 1);

Checking control.jump_to_label is one of the first things GuiType::Event does, so there isn't much need to check it here.

Lexikos commented on 8a9ade4 Jan 20, 2012

When checking for an empty string, the fastest way is to check just the first character. If it is a null character (zero), the string is empty; otherwise the string is not empty. Also, I presume you meant to check szUrl rather than szID.

if (*item.szUrl)  // URL is not empty.

What should happen if the URL cannot be executed? Currently I think nothing happens. Perhaps the g-label should be called so that fallback code can be written. For instance,

if (!*item.szUrl ||
    !g_script.ActionExec((LPTSTR)CStringTCharFromWCharIfNeeded(item.szUrl).GetString(), NULL, NULL, false))
    pgui->Event(control_index, nmhdr.code, GUI_EVENT_NORMAL, item.iLink + 1);

Checking control.jump_to_label is one of the first things GuiType::Event does, so there isn't much need to check it here.

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 20, 2012

Owner

I actually wanted to check szID since I noticed it is empty when an URL is used, but I guess it doesn't matter which one gets used.
I'll commit your changes, sounds good.

As you can probably tell I don't have too much C++ experience. I wrote some a few years ago, but I was not using much of the basic functions and character arrays then as higher-level classes were used.

I'm getting an error on GetString(), is that meant to be there? It compiles fine (On Unicode) without it.

Owner

ChrisS85 replied Jan 20, 2012

I actually wanted to check szID since I noticed it is empty when an URL is used, but I guess it doesn't matter which one gets used.
I'll commit your changes, sounds good.

As you can probably tell I don't have too much C++ experience. I wrote some a few years ago, but I was not using much of the basic functions and character arrays then as higher-level classes were used.

I'm getting an error on GetString(), is that meant to be there? It compiles fine (On Unicode) without it.

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 22, 2012

Isn't szID always empty unless you actually define the id="" attribute?

I thought it would be cleaner to use GetString() than relying on a type-cast to implicitly invoke the same function, but I'd forgotten that the macro does not construct a CString' object on the Unicode build. The following should be used instead:

(LPTSTR)(LPCTSTR)CStringTCharFromWCharIfNeeded(item.szUrl)

Both type-casts are required on the ANSI build: one to invoke GetString() and the other to remove the const qualifier.

Lexikos replied Jan 22, 2012

Isn't szID always empty unless you actually define the id="" attribute?

I thought it would be cleaner to use GetString() than relying on a type-cast to implicitly invoke the same function, but I'd forgotten that the macro does not construct a CString' object on the Unicode build. The following should be used instead:

(LPTSTR)(LPCTSTR)CStringTCharFromWCharIfNeeded(item.szUrl)

Both type-casts are required on the ANSI build: one to invoke GetString() and the other to remove the const qualifier.

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 22, 2012

Owner

Yes it's empty except for that case, but since there's only href and id tags it shouldn't matter which one to use.

Owner

ChrisS85 replied Jan 22, 2012

Yes it's empty except for that case, but since there's only href and id tags it shouldn't matter which one to use.

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 22, 2012

I don't quite understand what you're saying. We execute szUrl, not szID. It matters very much which one we check, since we want to call the g-label if href is omitted regardless of whether an id attribute was defined.

Lexikos replied Jan 22, 2012

I don't quite understand what you're saying. We execute szUrl, not szID. It matters very much which one we check, since we want to call the g-label if href is omitted regardless of whether an id attribute was defined.

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 22, 2012

Owner

I didn't think that one could specify tags without either an id or a href. Considering that checking szUrl is indeed better.

Owner

ChrisS85 replied Jan 22, 2012

I didn't think that one could specify tags without either an id or a href. Considering that checking szUrl is indeed better.

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 20, 2012

Owner

Can this be integrated in the event handling function so it is visible as event variable ...

Perhaps it can be stored in ErrorLevel. I think it's currently only used for ListViews. Search for gui_action_errorlevel in application.cpp.

Owner

Lexikos commented Jan 20, 2012

Can this be integrated in the event handling function so it is visible as event variable ...

Perhaps it can be stored in ErrorLevel. I think it's currently only used for ListViews. Search for gui_action_errorlevel in application.cpp.

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 22, 2012

The appropriate place to handle control events is just above this section, in the switch structure which contains case GUI_CONTROL_LISTVIEW:.

(However, now I see your comment about the size of gui_action_errorlevel.)

Try testing with a GuiClose: label; I suspect you'll get an access violation since pcontrol would be NULL.

Lexikos commented on 337b851 Jan 22, 2012

The appropriate place to handle control events is just above this section, in the switch structure which contains case GUI_CONTROL_LISTVIEW:.

(However, now I see your comment about the size of gui_action_errorlevel.)

Try testing with a GuiClose: label; I suspect you'll get an access violation since pcontrol would be NULL.

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 25, 2012

Owner

There's a problem with auto-sizing: the link markup is included in the calculations, so the control is wider than it should be. I tried using LM_GETIDEALSIZE after creating the control, but it doesn't seem to be accurate (particularly with non-default font). The only solution I can think of is to create a copy of the text minus the markup tags, and measure that. Would you like to take a crack at it? It should be adequate to simply strip out , <a ...> and . The most relevant section begins at line 2588 of script_gui.cpp.

Owner

Lexikos commented Jan 25, 2012

There's a problem with auto-sizing: the link markup is included in the calculations, so the control is wider than it should be. I tried using LM_GETIDEALSIZE after creating the control, but it doesn't seem to be accurate (particularly with non-default font). The only solution I can think of is to create a copy of the text minus the markup tags, and measure that. Would you like to take a crack at it? It should be adequate to simply strip out , <a ...> and . The most relevant section begins at line 2588 of script_gui.cpp.

@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 25, 2012

Contributor

I took a look at it, but I'm having some difficulties with C++ string functions. What are the suggested functions for parsing it? Or how to invoke the PCRE Regex replace function natively?

Contributor

ChrisS85 commented Jan 25, 2012

I took a look at it, but I'm having some difficulties with C++ string functions. What are the suggested functions for parsing it? Or how to invoke the PCRE Regex replace function natively?

@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 30, 2012

Contributor

I took another go at it and succeeded. It now parses all tags. If you think it needs to only parse "A" tags it should be easy to modify.

Contributor

ChrisS85 commented Jan 30, 2012

I took another go at it and succeeded. It now parses all tags. If you think it needs to only parse "A" tags it should be easy to modify.

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Jan 30, 2012

Owner

Only <a> tags are interpreted by the control. They aren't recognized if there is whitespace between "<" and "a" or on either side of "a" in </a>. Any attributes are acceptable, but if there they are not exactly in the form attribute="value", the tag isn't recognized. That means no whitespace around "=" and there must be quotation marks. There can be whitespace at the end of the opening tag.

Handling cases with markup errors correctly might seem unimportant, but if you treat an invalid tag as valid, some text will be cut off and it might be difficult to see the problem. There might also be cases where "<" and ">" are used literally but could be mistaken for a tag by the measuring code.

Unless the control has the LWS_NOPREFIX style, you also need to remove the first &, which is used the same way as a Text control. HTML entities don't seem to be supported.

Owner

Lexikos commented Jan 30, 2012

Only <a> tags are interpreted by the control. They aren't recognized if there is whitespace between "<" and "a" or on either side of "a" in </a>. Any attributes are acceptable, but if there they are not exactly in the form attribute="value", the tag isn't recognized. That means no whitespace around "=" and there must be quotation marks. There can be whitespace at the end of the opening tag.

Handling cases with markup errors correctly might seem unimportant, but if you treat an invalid tag as valid, some text will be cut off and it might be difficult to see the problem. There might also be cases where "<" and ">" are used literally but could be mistaken for a tag by the measuring code.

Unless the control has the LWS_NOPREFIX style, you also need to remove the first &, which is used the same way as a Text control. HTML entities don't seem to be supported.

@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Jan 31, 2012

Contributor

Ok I'll add enhanced parsing then.
Btw, when are you planning to release next AHK_L? I'm asking because I depend on the link controls for the next version of my program 7plus and it's mostly done.

Contributor

ChrisS85 commented Jan 31, 2012

Ok I'll add enhanced parsing then.
Btw, when are you planning to release next AHK_L? I'm asking because I depend on the link controls for the next version of my program 7plus and it's mostly done.

@Lexikos Lexikos merged commit 39d2abe into Lexikos:master Feb 1, 2012

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Feb 1, 2012

Owner

There were a couple minor errors and some inconsistencies between your parsing code and how the control actually handles things:

  • The last character of the link was replaced with ">".
  • LWS_NOPREFIX should be used in place of SS_NOPREFIX, which is only for Static controls.
  • Your code didn't actually do anything when it "found" the ampersand. It generally didn't need to do anything, since DrawText by default handles the first ampersand the same way as the control. However, adding the LWS_NOPREFIX style to the control put the size out slightly if an ampersand was present.
  • Tabs are tolerated within the <a> tag.
  • I've detailed much of what the SysLink control appears to allow in the comments in commit b6ce337.

Thanks for all your work. As you've probably seen, I've merged your code into the main branch for inclusion in the next release.

FYI, for(int i = 0, aChar = ... actually contains two declarations. Your code was declaring aChar and tagChar as TCHAR, then declaring new int variables with the same names.

Btw, when are you planning to release next AHK_L?

There is one small thing I've been meaning to do, but I'm mostly just waiting for you to write documentation for the Link control. ;)

Owner

Lexikos commented Feb 1, 2012

There were a couple minor errors and some inconsistencies between your parsing code and how the control actually handles things:

  • The last character of the link was replaced with ">".
  • LWS_NOPREFIX should be used in place of SS_NOPREFIX, which is only for Static controls.
  • Your code didn't actually do anything when it "found" the ampersand. It generally didn't need to do anything, since DrawText by default handles the first ampersand the same way as the control. However, adding the LWS_NOPREFIX style to the control put the size out slightly if an ampersand was present.
  • Tabs are tolerated within the <a> tag.
  • I've detailed much of what the SysLink control appears to allow in the comments in commit b6ce337.

Thanks for all your work. As you've probably seen, I've merged your code into the main branch for inclusion in the next release.

FYI, for(int i = 0, aChar = ... actually contains two declarations. Your code was declaring aChar and tagChar as TCHAR, then declaring new int variables with the same names.

Btw, when are you planning to release next AHK_L?

There is one small thing I've been meaning to do, but I'm mostly just waiting for you to write documentation for the Link control. ;)

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Feb 1, 2012

Owner

There is one small thing ...

I've done that small thing but remembered that I'm also waiting for Russel to write documentation for #InputLevel.

Owner

Lexikos commented Feb 1, 2012

There is one small thing ...

I've done that small thing but remembered that I'm also waiting for Russel to write documentation for #InputLevel.

@ChrisS85

This comment has been minimized.

Show comment
Hide comment
@ChrisS85

ChrisS85 Feb 1, 2012

Contributor

Now that you're saying it (declaration in for loop) it makes sense since C++ has scopes in for loops, didn't think about that at the time.

I started writing the documentation, but I wonder if I should remove the part in text controls about emulating link behavior? It's rather pointless now, but clickable text controls might be beneficial for other uses so it should maybe stay there with a reference to a better solution for links?

Contributor

ChrisS85 commented Feb 1, 2012

Now that you're saying it (declaration in for loop) it makes sense since C++ has scopes in for loops, didn't think about that at the time.

I started writing the documentation, but I wonder if I should remove the part in text controls about emulating link behavior? It's rather pointless now, but clickable text controls might be beneficial for other uses so it should maybe stay there with a reference to a better solution for links?

@Lexikos

This comment has been minimized.

Show comment
Hide comment
@Lexikos

Lexikos Feb 2, 2012

Owner

Leave it in. Rather than simply adding a reference to link controls, it might be more effective to add a link control to the example. A user can run the example to compare the functionality and syntax of the two. There might be cases where a text control is preferred - for instance, to apply a custom colour while also making the whole text clickable.

Owner

Lexikos commented Feb 2, 2012

Leave it in. Rather than simply adding a reference to link controls, it might be more effective to add a link control to the example. A user can run the example to compare the functionality and syntax of the two. There might be cases where a text control is preferred - for instance, to apply a custom colour while also making the whole text clickable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment