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

Fix dnd with chrome #156

Merged
merged 4 commits into from
Feb 10, 2021
Merged

Fix dnd with chrome #156

merged 4 commits into from
Feb 10, 2021

Conversation

okratitan
Copy link
Contributor

This fixes dnd with chrome and fixes #49

@okratitan okratitan requested a review from andydotxyz February 8, 2021 18:57
@okratitan
Copy link
Contributor Author

Before approving we should discuss if reversing the stack list is correct - or if we should use the bottom to top order in stack list to begin with. I didn't make this change yet because I couldn't remember if there a reason we went to top to bottom on stack list.

@andydotxyz
Copy link
Contributor

Great catch. As for the ordering I am not sure. I think the window order was based on the order we would display them on switcher. So either we reverse it here, or in the switcher code. We should probably document the order in the Stack interface I guess.

andydotxyz
andydotxyz previously approved these changes Feb 8, 2021
wm.go Outdated
@@ -14,13 +14,13 @@ type WindowManager interface {
}

// Stack describes an ordered list of windows.
// The order of the windows in this list matches the stacking order on screen.
// TopWindow() returns the 0th element with each item after that being stacked below the previous.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, I forgot that we defined it this way around.
Are we certain that reversing it for all platforms is the right thing to do?

Copy link
Contributor Author

@okratitan okratitan Feb 9, 2021

Choose a reason for hiding this comment

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

I can't see any reason why it wouldn't be fine for any other platform -- I'm not certain though. However I'm less certain that having it top to bottom is the right thing to do. So far we have one implementation - X and it says bottom to top. That should be fine for Wayland from what I can tell so far too... I would say between the two there isn't a lot of certainty of which one is better - but the one concrete thing we have and know is how the one platform we have wants it bottom to top and from a code stand point that means less conversions and slice walking so I think that tips that scales for me.

@andydotxyz andydotxyz dismissed their stale review February 9, 2021 13:49

Code changes should have removed approval

@andydotxyz
Copy link
Contributor

I jumped over to WinGo as an example of another WM in go - they have stacked it how we previously did:

https://github.com/BurntSushi/wingo/blob/30b336cbb88d32c12c14768d5ff89b3d55560081/stack/stack.go

@okratitan
Copy link
Contributor Author

okratitan commented Feb 9, 2021

Yes and they had to provide a helper function to convert it. Walking the slice to convert the stack changes on every focus, raise, lower, add, and remove of windows surely isn't ideal, and appending new windows or raised windows to the end of a slice surely makes more sense than inserting them at the beginning.

@andydotxyz
Copy link
Contributor

If The X11 driver code wants to use the reverse order for performance reasons then that is fair enough.
But that in itself does not indicate that we should reverse the exposed API...

Another approach would be that the Windows method could reverse it so that the backwards list is internal to the driver only and our API remains unaltered.

@okratitan
Copy link
Contributor Author

okratitan commented Feb 10, 2021

Fair enough - I reverted the interface/switcher changes and made the change to reverse the stack on Windows() method

@okratitan okratitan requested a review from andydotxyz February 10, 2021 14:41
Copy link
Contributor

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks, this feels like a nice clean change and a very welcome fix :)

@okratitan okratitan merged commit 57dc8e7 into FyshOS:develop Feb 10, 2021
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

Successfully merging this pull request may close these issues.

2 participants