On Sat, 12 Aug 2017 03:45:39 +0200 Roman Gilg <[email protected]> wrote:
> On Thu, Aug 10, 2017 at 2:49 PM, Pekka Paalanen <[email protected]> wrote: > > Assuming the theory is sound, how about you kept the old names and > > semantics of xwl_window_get() and xwl_window_from_window() but just > > used the stored private in xwl_window_from_window()? You would need to > > have a xwl_window_find_from_window() for the single use in > > xwl_realize_window(), but I think that would allow you to put the test > > > > if (win != xwl_window->window) > > > > in xwl_window_get() rather than open-coding it in the users. I believe > > keeping the semantics would be less surprising to the reader, > > especially if they know the old code. > > > > It's a good idea to not open-code the condition for the users. I would > prefer though to risk the short-term confusion for the users, because > otherwise in the long term the naming is ambiguous: > > A Window has exactly one xwl_window associated (or none): The one in its > private field. Calling xwl_window_get() therefore is the natural way to > receive this one xwl_window. The function naming becomes really confusing > when we call xwl_window_from_window() inside xwl_window_get() to receive > the private field and afterwards do the test or when we call > xwl_window_from_window() in xwl_window_find_from_window() on the Window's > parent Window. Hi Roman, are you seeing xwl_window_get() as "get the private" while I think of it as "get the owned xwl_window"? In that case, and as it has not been documented which one it really meant before, I think we should just not use the name xwl_window_get() anymore at all to get rid of the ambiguity. This is the most important point in my comments and even that is not too important. > How do you like this approach: > * xwl_window_get(WindowPtr) returns the private field > * New function xwl_window_own(WindowPtr win) calls xwl_window_get and > forwards the xwl_window if (win == xwl_window->window), otherwise NULL. > Maybe there is a better name than xwl_window_own though for this function? > * xwl_window_from_window() is renamed to xwl_window_find() and as it's now > will only be called in xwl_realize_window. > > But if you prefer the other naming, it's also fine with me. Yes, that sounds better. Since the semantics of xwl_window_get() change due to setting the window private for more windows than the one actually owning the xwl_window, it requires touching all the current callers. That's why I previously suggested to keep its semantics intact. Would it be nice to rename xwl_window_get() to xwl_window_from_priv() to denote this change in semantics and ensure that no callers go unnoticed? Or to_xwl_window()? xwl_window_own() would be the replacement for the old xwl_window_get(), or maybe call it... xwl_window_from_owner()? Returns NULL if the Window doesn't own any xwl_window. How does your Present and sub-surface work change the relationships between Windows and xwl_windows? Does the child Window getting a sub-surface have its own xwl_window? Are there cases for fetching from the child Window (or from its descendant) both its own xwl_window and the respective toplevel Window's xwl_window? If the above doesn't feel particularly fitting, then let's go with your suggestion. Thanks, pq
pgpTf0C51NLEN.pgp
Description: OpenPGP digital signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
