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

WIP: Label inherits View and clean event handling code #450

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yostane
Copy link
Contributor

@yostane yostane commented Apr 8, 2021

Fixes: #442

@compnerd
Copy link
Owner

compnerd commented Apr 8, 2021

Thanks for the patch @yostane! Does this not break the current mechanism for handling events? I think that the long term solution here is to use gesture recognizers and attach one to the button, but for the mean time, it would be nice to have something that continues to function.

@yostane
Copy link
Contributor Author

yostane commented Apr 8, 2021

Hi you're welcome :).

Ah ok I understand. What should be done is:

  • Make Label inherit from View
  • Keep the current event handling mechanism (because GestureRecognizer is not implemented yet)

If that's what is expected, I will update my patch if possible.

@compnerd
Copy link
Owner

compnerd commented Apr 8, 2021

Yeah, I think that would be the best middle ground where we dont have the desired state, but keep the ability to detect the click. I think that follow ups for adding GestureRecognizers would be wonderful :)

@compnerd compnerd added the enhancement New feature or request label Apr 9, 2021
@compnerd compnerd self-requested a review April 9, 2021 21:46
@yostane yostane changed the title Label inherits View and clean event handling code WIP: Label inherits View and clean event handling code Apr 14, 2021
@yostane
Copy link
Contributor Author

yostane commented Apr 14, 2021

Hi @compnerd . Can you take a look at my last modifications please ?

I transferred some event handling code from Control into Label to allow the developer to use the addTarget method on the Label. I have added a click handler on the the Label of the HelloWorld example and it responds to the click.
Is this how it should be done ? It that's ok as a temporary solution, I will remove some test code as well as the the WIP status

Thanks,

Copy link
Owner

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does add a lot of code to Label - and it seems most of it can be extracted into an extension. What do you think of pulling it all out into an extension and putting it into a separate file (say Label+Temporary.swift? I do have some of the work for the gesture recognizer, we should be able to start considering adding the TapGestureRecognizer in place instead.

}
}

private let SwiftLabelProc: SUBCLASSPROC = { (hWnd, uMsg, wParam, lParam, uIdSubclass, dwRefData) in
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move this below the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, I put it back in the wrong location 😁


let size = self.frame.size
self.hWnd_ = CreateWindowExW(0, WC_STATIC.wide, nil, DWORD(WS_CHILD),
0, 0, CInt(size.width), CInt(size.height),
self.hWnd, nil, GetModuleHandleW(nil), nil)!

_ = SetWindowSubclass(hWnd, SwiftLabelProc, UINT_PTR(1),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason for this move?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, I put it back in the wrong location 😁

@yostane
Copy link
Contributor Author

yostane commented Apr 27, 2021

I tried moving code in Label+Temporary but this seems more complex than expected because of visibility problems. The current Laabel+Temporary file contains code that did not require changes.

Instead of spending more time on this temporary solution, maybe it would be better to postpone this MR and focus more on gesture recognizer. What do you think ?

@compnerd
Copy link
Owner

Postponing and focusing on gesture recognizer sounds like a good plan to me. @egorzhdan had previously looked at the gesture recognizer bits for menus, not sure if he has any thoughts about what exactly would be the minimum work needed to get it working.

@egorzhdan
Copy link
Contributor

Yeah, I've looked into handling menu item clicks before, but I don't have a working implementation of it.
IIUC for menus that would require handling WM_COMMAND events & deducing the menu item that was clicked based on LOWORD(wParam), but for labels the event could be different.

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

Successfully merging this pull request may close these issues.

Views and Controls: Label should derive from View
3 participants