Mir

Code review comment for lp:~vanvugt/mir/managed-surface

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Alan:
I share your concerns and agree with your statements. If there's a way for us to get to a cleaner design, then I don't mind implementing surface management logic a 4th/5th/6th time. I would like us to be able to land some small first steps, somehow.

Robert:
I think your arguments lack objective justification:

3. OK, demonstrate it in a way that does not make things more complex, again.

3b. Flexibility is a good thing. I aim for code reuse to make things easier in the future. And this has been proven successful in the code I have landed in Mir over the past 2 years.

3c. I am not "inventing a problem". The problem is real as firstly demonstrated below (ManagedSurface contains restore_rect, and in future more such data like minimize_rect). Secondly, it's been clear for a long time that one cannot implement animations for example without storing animation state somewhere. And you agree "somewhere" is required. Your suggested "somewhere" of association is a hack that is inelegant and difficult to maintain. Because a separate associative container not only needs to be kept in sync with the real list of surfaces, but also notified of all surface changes (ManagedSurface solves both these problems easily).

3d. The tiling branch shouldn't come into the discussion yet. It doesn't solve the same problems as this branch does. I remind you of the requirements I seek to fulfil:

  1. libmirserver implements some sane default window management policy, so that shells don't need to reinvent the wheel each time.
  2. libmirserver needs to provide shells with a convenient mechanism of attaching their own member variables to each surface. Without the overhead of extra association containers to keep in sync.

This branch solves both while the tiling branches solve neither (yet).

But a single branch doesn't need to solve both. If it makes people happier, I can divide this one up into smaller pieces.

« Back to merge proposal