Inset Mac OS traffic lights

[haasal]

What kind of change does this PR introduce?

• [ ] Bugfix
• [x] Feature
• [ ] Docs
• [x] New Binding issue tauri-apps/tauri#4789
• [ ] Code style update
• [ ] Refactor
• [ ] Build-related changes
• [ ] Other, please describe:

Does this PR introduce a breaking change?

• [ ] Yes, and the changes were approved in issue #___
• [x] No

Checklist

• [x] When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
• [ ] A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
• [x] I have added a convincing reason for adding this feature, if necessary

Other information

Important: There still exist artifacts when resizing the window because the traffic lights are set just after the window has already been drawn.

[morajabi]

Thanks for this awesome PR! I had tried doing this only to fail at the "resize" artifacts: #494 Reading your solution made it clear. I should close that issue then.

[PhotonQuantum]

This feature is fantastic!

However, I noticed a strange behavior: the hover effect still shows when I move the cursor to the "original" position of traffic lights. The electron implementation doesn't seem like have this behavior. I dug into its code but failed to find a solution.

Update: strangely, the problem goes away if I resize the window.

[tr3ysmith]

Hey what's the status of this? I really would like to be able to apply inset on the traffic lights without work arounds

[haasal]

Hi, me too. But I know really little about Tao. If this gets merged then we still have to implement it somehow into tauri. Could someone review this please?

[wusyong]

The implementation seems fine to me. But I'm not sure if it's good to merge now. @amrbashir Do you think winit can accept this? Btw you can get the raw window handle to change it yourself if you can't wait for it any longer.

[amrbashir]

there is a good chance they may not accept but lets go ahead and merge this and if they didn't accept it upstream, we can implement it directly in tauri

[haasal]

I'm sorry this has taken so long. I didn't really have time to work on this. I will document the function and delete the example these days. I hope it's not to late to merge?

[amrbashir]

Please rebase the PR to fix the conflicts and add a change file

[JonasKruckenberg]

I'll review this tomorrow, it's a super important imo and this or has been open wayy too long 😉 let's get this over the finish line

[haasal]

Maybe it's important to add that I couldn't test the changes since my MacBook broke 🥺. If one of you could do that it would be amazing

[JonasKruckenberg]

After testing this PR now, I feel LogicalSize makes no sense here, the way you currently call the method is as such

                                      // WTH means width here? and height?
.with_traffic_light_inset(LogicalSize { width: 10, height: 20 });

[JonasKruckenberg]

Okay a few other observations:

1. While this does successfully inset the traffic lights, it does not increase the draggable area size
2. When resizing the window, it looses the inset

The second one really is a dealbreaker right now, I guess we need to implement a delegate and stuff just like electron does as well

[haasal]

Okay a few other observations:

1. While this does successfully inset the traffic lights, it does not increase the draggable area size

2. When resizing the window, it looses the inset

The second one really is a dealbreaker right now, I guess we need to implement a delegate and stuff just like electron does as well

I don't know if you fixed it already in that commit, but when I tested it with my supplied example the inset was persistent during resizing of the window.

[JonasKruckenberg]

I don't know if you fixed it already in that commit, but when I tested it with my supplied example the inset was persistent during resizing of the window.

Okay yes my bad, calling the set_traffic_light_inset method after the RedrawRequested event got fired fixes the artifacts.

My only question @haasal is why the with_traffic_light_inset method exists if it's not really doing what the user expects. This feels like misleading api design

[JonasKruckenberg]

@amrbashir I added back the example, since it's indeed needed to use the method correctly (it's not enough to just call it once but it needs to be called after RedrawRequested)

[haasal]

Yeah that was the reason I made the example. I already tried implementing this in Tauri itself using the raw window handle but I couldn't get rid of these artifacts. Do you think this method can now be used in tauri without causing artifacts?

[haasal]

My only question @haasal is why the with_traffic_light_inset method exists if it's not really doing what the user expects. This feels like misleading api design

I don't know how to implement it instead. This whole thing gave me big headaches because I couldn't find a way to set the inset without having to call the method in some weird way. If you have any idea how to do this without having to call the method after RedrawRequested it would be great!

[amrbashir]

I still don't see why the example is needed since tao should already handle that internally and not require users to keep calling it in RedrawRequested

[JonasKruckenberg]

I still don't see why the example is needed since tao should already handle that internally and not require users to keep calling it in RedrawRequested

then we should change it to behave that way

[haasal]

I really would like to get this over with but as said earlier I don't really know how. If you know how to directly inject something into the "window draw pipeline" (I.e. how to automatically call my inset-method/function without requiring the user to do some weird setup) I could take a look at it

[amrbashir]

@haasal you need add an inset field on the ViewState struct here https://github.com/tauri-apps/tao/blob/c5d606dffeb1733ab06fd8c43eb3b9e7b2f553fe/src/platform_impl/macos/view.rs#L54 and your new function set_traffic_light_inset sets this field, then in draw_rect https://github.com/tauri-apps/tao/blob/c5d606dffeb1733ab06fd8c43eb3b9e7b2f553fe/src/platform_impl/macos/view.rs#L382 you have access to the state where you can read it and re-position the tarffic lights

[haasal]

But the with_traffic_light_inset() method should remain as the public API method for the user to call, right? The user just shouldn't ever have to call set_traffic_light_inset() manually.

[amrbashir]

@haasal No, both methods should be public, both should modify the newly added state and triggers the traffic light positioning

[haasal]

Sorry for not contributing to this in the last weeks. In spite of what I said I noticed that I don't currently have the time to finish this. I would really appreciate if one of you could push this over the finish line.

[haasal]

Got a new mac and fixed it. I removed the set_traffic_light_inset() method because it wasn't really necessary anymore and would add unnecessary complexity. The with_traffic_light_inset() method on WindowBuilder should now work as expected.

Edit: I added the set_traffic_light_inset() back in. I overestimated how complicated it would be to implement this

[haasal]

Imho apart from minor refactoring (if you see required) this should be ready to merge. Could you maybe take a look at this @JonasKruckenberg @amrbashir?

[haasal]

@amrbashir ok, I implemented your feedback. I hope the LogicalPosition is ok

[amrbashir]

Thank you

[amrbashir]

Just waiting for @wusyong to test and approve as well.

[amrbashir]

@haasal looks like you didn't sign your commits and I can't merge the PR unless they are signed. You need to setup commit signing, then you can sign past commit like this for example.