Add support for ext-foreign-toplevel-list-v1 #2833
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ammen99
reviewed
Sep 10, 2025
Contributor
Author
|
I am marking this as draft because I seem to encounter a crash when both plugins are active. Just closing a toplevel is sufficient to trigger this crash. I think there is some memory corruption that's taking place. I'll mark this as ready once I can fix the crash. @ammen99 Here is the ASAN output if it helps: |
Member
|
@marcusbritanicus I think you are trying to reuse too much between the plugins, I meant simply the app-id generation function. If you truly want to reuse the rest as well, I think you will need to use something like the strategy pattern:
What you have currently definitely is not a good idea if you have both plugins enabled: they implement the same class in two different ways, that is generally not allowed and will lead to crashes. Considering that there is also ext-toplevel-management, I think it is worth thinking how it will play with these abstractions ... Maybe we need to split the big shared class into two parts, each responsible for either the listing or for the management. The wlr protocol implementation then would simply instantiate both shared classes, and the ext protocols will instantiate one of each. |
Contributor
Author
|
@ammen99 Thanks!! I was under the (incorrect) impression that since they're different plugins it will not matter. Anyways, I decided to make subclasses for wlr and ext plugins. All the junk values I was getting in the previous tests are now gone. I have tested for both ext-foreign-toplevel-list and wlr-foreign-toplevel-management protocols. Both work seamlessly and no crashes. Please have a look and tell me if is what you meant. |
ammen99
requested changes
Sep 24, 2025
Contributor
Author
|
@ammen99 Incorporated all your suggestions. They were all murmurs from the old code. I have also rebased - I think I have done it properly. Please do check and tell me if everything is proper. |
Member
|
Something seems to have gone wrong in the rebase, github reports many more changes in this PR than actually intended .. |
marcusbritanicus
force-pushed
the
ext-toplevel-list
branch
from
0091baf to
4835cfb
Compare
Contributor
Author
|
@ammen99 I force-pushed the changes on top the latest master. I think now everything should be proper. |
ammen99
reviewed
Oct 16, 2025
| toplevel_send_state(); | ||
| }; | ||
|
|
||
| wf::wl_listener_wrapper toplevel_handle_v1_maximize_request; |
Member
There was a problem hiding this comment.
I think that just like the void *handle field, these belong in the concrete implementations, as they are not used in the base class at all. I suppose we could also drop the function for initializing them from the interface.
Rationale: I get what you like with the current implementation, it seems that we share some code and code structure. But in reality I feel that by adding these abstractions here, we are simply removing 'duplicate' code by adding complex abstractions which are not 'natural' and make the code hard to read (to understand what is going on you have to go back and forth between the base and the derived classes).
A bit late, but if you are really keen on perfecting this, I would even suggest using the strategy design pattern. As people say, 'prefer composition over inheritance'. Have the base class which connects to signals, then pass a pointer to an interface implementation in the constructor. The interface would contain just the methods that are to be overridden (send_initial_state, toplevel_send_state, etc.). That would be best from a design perspective, but as we have been going back and forth I would also be ok with the current state, just move the listener_wrappers and their initialization functions to the derived classes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@ammen99 I did an initial test of this plugin. I am able to list the toplevels, and get their titles and app-ids, which is what this is supposed to do. Please let me know if there are any changes required.