[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 119944: Remove UI assumptions about the alternatives dialog
From: "Aaron J. Seigo" <aseigo () kde ! org>
Date: 2014-08-26 19:45:38
Message-ID: 20140826194538.11205.62761 () probe ! kde ! org
[Download RAW message or body]
--===============4337496733181382041==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119944/
-----------------------------------------------------------
(Updated Aug. 26, 2014, 7:45 p.m.)
Status
------
This change has been marked as submitted.
Review request for Plasma.
Repository: plasma-workspace
Description
-------
This changes AlternativesDialog to AlternativesHelper that only exports the necessary \
functionality, and puts the onus on the shell package to provide the proper UI. The \
alternatives QML is expected to provide a bool visibile property with a \
visibleChanged signal to allow proper cleanup. This will be documented in the \
plasmashell documentation that I am also working on.
This happens to remove the need for plasmaquick's Dialog class in plasmashell. For \
those who haven't noticed: libplasmaquick is not providing binary compatibility \
guarantees and is accessed by *private headers copied over into repositories*; seeing \
as one package is released with KDE Frameworks and the other Plasma Workspace, this \
is obviously a binary compatibility disaster just waiting to happen and quite \
obviously violates the contract between Frameworks and Workspace being different \
products. This patch set also introduces the need to use plasmaquick/ in the #include \
directives so it is easy to see where the trouble spots are.
The other motivation for this change is that providing an *applet-positioned dialog* \
is a gross assumption about the hardware form factor plasmashell is running on. The \
entire point of having plasmashell was to have a single shell that can be used on \
different form factors and with different shell packages that dictate the results, \
yet such assumptions have crept in one by one.
This patch set is one small step to addressing these issues which currently prevent \
plasmashell from being a serious candidate for use on non-desktop/laptop form \
factors.
Diffs
-----
shell/interactiveconsole.cpp e7a3e85e0119ea6fa22a293ec226ae23005e9b2c
shell/main.cpp e34578d8142e95362734fdd4db9d06da7fc02b20
shell/osd.cpp a2bee29f400f8c4aca690205f55699bae185b647
shell/panelconfigview.h 02e1264572d4b9b744cd6d4aaf1df694f3e4aa6d
shell/panelview.h bd1643813bb182c5faaf4682fbd4b7adb61979a7
shell/plasmaquick/dialog.h 6759a6ea37472a71e291e3bfaab0d7a82afba323
shell/shellcorona.h bb9f0c1110887b372a4e084a64abb0fabfa1bc5b
shell/shellcorona.cpp 2b8389ee54b74bfe84ad685d145f9dabb5b24a8f
shell/CMakeLists.txt 700d08b419918b9863c44691473cd4f9cae00d86
shell/alternativesdialog.h 195b69a304d7708afbe127928b2a0e129880f281
shell/alternativesdialog.cpp 56d686ded958bfa7de2579e4c8dce04d3771d615
shell/containmentconfigview.h 8c6c2a2444c0e031cb21876a56ae3b861b01e902
shell/containmentconfigview.cpp acfa6b24f11beed00b81b557b39dcf6f39eedcf6
shell/desktopview.h 620d6df5a2fa516b9d9fc59184f2d9c7669efc5f
Diff: https://git.reviewboard.kde.org/r/119944/diff/
Testing
-------
Run with a modified desktop shell package and alternatives come up as expected and \
the QmlObjects are cleaned up appropriately as well.
Thanks,
Aaron J. Seigo
--===============4337496733181382041==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119944/">https://git.reviewboard.kde.org/r/119944/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="12" style="border: 1px gray solid; \
border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> <tr>
<td>
<h1 style="margin: 0; padding: 0; font-size: 10pt;">This change has been marked as \
submitted.</h1> </td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for Plasma.</div>
<div>By Aaron J. Seigo.</div>
<p style="color: grey;"><i>Updated Aug. 26, 2014, 7:45 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This changes AlternativesDialog to AlternativesHelper \
that only exports the necessary functionality, and puts the onus on the shell package \
to provide the proper UI. The alternatives QML is expected to provide a bool visibile \
property with a visibleChanged signal to allow proper cleanup. This will be \
documented in the plasmashell documentation that I am also working on.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">This happens to remove the need for plasmaquick's Dialog class in \
plasmashell. For those who haven't noticed: libplasmaquick is not providing binary \
compatibility guarantees and is accessed by <em style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">private headers copied \
over into repositories</em>; seeing as one package is released with KDE Frameworks \
and the other Plasma Workspace, this is obviously a binary compatibility disaster \
just waiting to happen and quite obviously violates the contract between Frameworks \
and Workspace being different products. This patch set also introduces the need to \
use plasmaquick/ in the #include directives so it is easy to see where the trouble \
spots are. </p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The other motivation for this change is that providing \
an <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">applet-positioned dialog</em> is a gross assumption \
about the hardware form factor plasmashell is running on. The entire point of having \
plasmashell was to have a single shell that can be used on different form factors and \
with different shell packages that dictate the results, yet such assumptions have \
crept in one by one.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">This patch set is one small step to \
addressing these issues which currently prevent plasmashell from being a serious \
candidate for use on non-desktop/laptop form factors.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Run with a modified desktop shell package and \
alternatives come up as expected and the QmlObjects are cleaned up appropriately as \
well.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>shell/interactiveconsole.cpp <span style="color: \
grey">(e7a3e85e0119ea6fa22a293ec226ae23005e9b2c)</span></li>
<li>shell/main.cpp <span style="color: \
grey">(e34578d8142e95362734fdd4db9d06da7fc02b20)</span></li>
<li>shell/osd.cpp <span style="color: \
grey">(a2bee29f400f8c4aca690205f55699bae185b647)</span></li>
<li>shell/panelconfigview.h <span style="color: \
grey">(02e1264572d4b9b744cd6d4aaf1df694f3e4aa6d)</span></li>
<li>shell/panelview.h <span style="color: \
grey">(bd1643813bb182c5faaf4682fbd4b7adb61979a7)</span></li>
<li>shell/plasmaquick/dialog.h <span style="color: \
grey">(6759a6ea37472a71e291e3bfaab0d7a82afba323)</span></li>
<li>shell/shellcorona.h <span style="color: \
grey">(bb9f0c1110887b372a4e084a64abb0fabfa1bc5b)</span></li>
<li>shell/shellcorona.cpp <span style="color: \
grey">(2b8389ee54b74bfe84ad685d145f9dabb5b24a8f)</span></li>
<li>shell/CMakeLists.txt <span style="color: \
grey">(700d08b419918b9863c44691473cd4f9cae00d86)</span></li>
<li>shell/alternativesdialog.h <span style="color: \
grey">(195b69a304d7708afbe127928b2a0e129880f281)</span></li>
<li>shell/alternativesdialog.cpp <span style="color: \
grey">(56d686ded958bfa7de2579e4c8dce04d3771d615)</span></li>
<li>shell/containmentconfigview.h <span style="color: \
grey">(8c6c2a2444c0e031cb21876a56ae3b861b01e902)</span></li>
<li>shell/containmentconfigview.cpp <span style="color: \
grey">(acfa6b24f11beed00b81b557b39dcf6f39eedcf6)</span></li>
<li>shell/desktopview.h <span style="color: \
grey">(620d6df5a2fa516b9d9fc59184f2d9c7669efc5f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119944/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============4337496733181382041==--
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic