On Thu, 30 Jun 2016 12:45:51 -0700 Bryce Harrington <[email protected]> wrote:
> On Thu, Jun 30, 2016 at 02:13:45PM +0300, Pekka Paalanen wrote: > > Hi Bryce, > > > > both pushed: > > a5bb91d..24f917e master -> master > > > > > > The activate_signal will be a lot more difficult to handle. The only > > in-tree user of it is xwayland's weston_wm_window_activate() which > > assumes there can only be one surface active at a time, and it's not > > even per seat, it's really just one. There might also be other users > > out there I do not know of. > > I was hoping in breaking this patch out separately that you'd see that > I'm not introducing this signal or using it for inhibition; it's > existing code, and doesn't need tinkered with beyond this rename. > > Yes, the signal is mis-named but that's a legacy issue unrelated to this > patch. Weston is rife with poorly chosen names already, so this is > nothing particularly out of the ordinary. Ideally there'd be some > documentation explaining what it is, but again, its lack of > documentation is hardly unique either. And unless there's a clear > *technical* reason why I'd need to tinker with it as part of the > inhibition work, I don't want to scope creep this patchset to deal with > either of those problems. Hi Bryce, there is a reason to tinker with it, and I explained it below, but only if you insist on tracking surface activeness in weston core. > > If we go with your plan to be able to have arbitrary many surfaces > > active at the same time, without changes XWM would raise and activate > > the most recent surface set active, regardless of what is already > > active. Is that desireable, I do not know. > > This is the second time you've asserted that this is "my plan". The patch has your name in it, and no-one else's. > > To be able to support any other kind of behaviour, we'd need signals for > > both activate and deactivate. Or, we have to decide that you can only > > have one surface active per seat, > > I have no problem with limiting it to one active surface per seat. > Indeed I was leaning that way to begin with by tracking input activity > on a per-seat basis but you've balked at that. I still think that's the > better way to go. Active per seat is a good idea, because of its close relationship to keyboard focus. It has nothing to do with tracking input activity. I also cannot see why idle-inhibit would need any tracking per seat whatsoever. > > in which case we'd have a per-seat > > activate_signal, and the active surface would be tracked just like > > keyboard focus except in struct weston_seat, not in struct > > weston_keyboard. > > I would need better convincing that signals need to be involved. To me > this seems like not needing to be so sophisticated a production. I'm > not opposed to using signals here but this is not that elaborate of a > feature that it needs to be turned into a major production just to get > implemented. And none of this is protocol, it can be refined later > after the basics have landed. There is the activate_signal. That needs to become per-seat, if you track the active surface per seat. > > Looks like all of weston_keyboard, weston_pointer, and weston_touch > > have their own focus_signals, too, which are naturally conditional on > > having the capability in the seat. > > > > These are the design questions we need to solve to add activeness > > tracking in Weston core. If we put "active" as a thing in the core, > > how should it behave? > > > > Therefore I'd suggest to reconsider "activeness" for idle-inhibit. If > > you just want a shell-controllable surface flag for "may inhibit idle > > triggers", then we should not talk about active because active is > > something else. > > I would quote you as being the one that directed me on this path to > begin with, but that was a private discussion. Needless to say, I'd > like to finish this implementation and get it landed rather than start > over from scratch yet again. I believe you have the right to quote private discussions in public if you see it as appropriate. I have given you a suggestion on how to land the idle-inhibit. Please use it. I reiterate that suggestion below. > If you can suggest a better name than "active" I'm all ears, but there > needs to be a way to distinguish between surfaces the user is actually > interacting with (and thus allowed to manipulate the screensaver), and > backgrounded/minimized/unfocused surfaces that shouldn't. The design This is where we disagree. In my opinion, both focused and unfocused surfaces (or active vs. inactive) should be able to inhibit on the same terms. Therefore I do not see a reason to track "activeness" in weston core, as I have repeatedly said. When you don't need to care about "activeness", we don't need to fight over what it means either, and you can leave activate_signal as is. Backgrounded (occluded?) or minimized is a completely different thing. Minimized surfaces are not in view_list, which means they will not be considered at all during output repaints or in your weston_output_inhibited_outputs(), which means they automatically lose their ability to inhibit. You don't have to add anything special to make that happen. If backgrounded means "completely occluded", that we do not have readily available, and shells do not know it either. That information must be extracted in the output repaint path. I think that can be left for later. Using the view_list to find inhibiting surface already automatically covers most cases, where you don't want a surface with an inhibitor to inhibit: windows that are minimized, hidden, on a non-visible virtual desktop, etc. Furthermore, using the view->output_mask like you did will also automatically exclude all surfaces that are outside of any outputs. The only thing not handled by this is occlusions, which must be computed from the scenegraph with the help of opaque regions. Your patches to implement idle-inhibit are almost ready. You only need to drop the unneeded patches! > I'm implementing is the one you suggested to me, and as the > implementation is mostly done (IMHO) suggestions to rethink that part of > the design is going to require tossing out a lot of time investment and > starting over, which obviously I'd like to avoid. Again, a lot of this > is just internal implementation details that can be refined later; I > would like to just get this landed. I believe we can get idle-inhibit in its basic form landed pretty easily, if you just drop the "active" stuff completely. Remove the use of weston_surface::active, and drop the patches adding it. I still stand by my claim, that you asked how to track active surfaces and I explained exactly that to you. You had *already* decided you need it for something, and I didn't care what you needed it for. I just explained the proper way to get it in my opinion. > > Then I'd also ask why is it a surface and not a view. > > Coincidentally (and evidently you've forgotten) but I asked that exact > same question of you at the inception of this; surfaces were the result > of that discussion. Unless there's a particular *tangible* scenario Are you sure you were not asking in the context of "activeness"? "Active" is a property that is visible to the client somehow (keyboard focus and shell-specific protocols), therefore it must be on the surface. A flag that only considers idle-inhibiting would be per-view, because you might have different kind of views that are not all equal wrt. inhibiting. > where inhibition should be different for one view of a client's surface > vs. another, changing the design at this point just causes excessive > work to reimplement everything. I'm not opposed to making it view-based > instead of surface-based if a compelling case could be made for why to > do it that way, but surface-based is going to cover all use cases I can > think of and will be simpler (and thus less error-prone). A tangible scenario: live thumbnail views. But Weston does not do that itself, you can perfectly leave that fight for the time when someone needs it. There are more things to do to have nice live thumbnail views anyway. You can ignore that for now. You don't need to reimplement everything. Just drop the things we don't need, we can get the feature landed, and practically all users will be happy using it. > Changing from surface to view later on would yes, unfortunately be a > protocol breakage, but the idle inhibit protocol is intentionally being > described as unstable and versioned so we're already giving ourselves > latitude for changing that later if needed. > > Pekka, I appreciate your review attention and thought to making this > design better, but at this stage what I really need is support in > getting this *landed*. In that case, please do as I suggested: drop all uses of any new flags, activeness or alike. That is why I have been recently stressing so hard on the question of why do you need activeness at all. We've been fighting over a thing you don't need, but rather than just flat out saying "drop that shit, it's not useful", I gave you the opportunity to explain why you want it, in case I just didn't understand. You ignored the question and continued on the details of how, assuming idle-inhibit cannot live without inspecting "active". But now it seems there really is no need for activeness tracking in weston core at all, so just drop that part of the series and be happy you don't have to fight with it anymore. The work lost is lost, now cut your losses and get the idle-inhibit feature in a shape where I can land it. Please. I would really like to land it. Now that I read again my reviews, it seems there were more questions about why do something at all (patch 1). I do not understand why you carry patches adding things you never use. How about you keep patch 3, the protocol implementation from patch 6, and patches 7 and 8, and drop all the rest for a start? Addressing the review comments for what remains shouldn't be anyway near as hard as this. Thanks, pq
pgpwL1jam2cgT.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
