8bb9768 · unity8 · git.iko.soy

git.iko.soy/unity8

Various fixes and refactoring

- append new surfaces instead of prepend them. This is what ApplicationManager does and what PhoneStage expects in order to get its new-window animation working - Work around issue in repeater when you add and immediately move a row in the model - Sort out mess when switching stages - Fix some tests

commit8bb976860476a6daefa20ca8bde070589534ecb0 authorDaniel d'Andrada <daniel.dandrada@canonical.com> dateMon 4 Apr 2016, 10:39 (11 years ago) parent86231ce

10 files changed+198−62

compare with xenial

unifiedsplit

plugins/WindowManager/TopLevelSurfaceList.cpp+82−24 plugins/WindowManager/TopLevelSurfaceList.h+19−6 qml/Shell.qml+4 qml/Stages/DesktopStage.qml+13−6 qml/Stages/PhoneStage.qml+17−6 qml/Stages/TabletStage.qml+8−12 qml/Stages/TopLevelSurfaceRepeater.qml+52 tests/mocks/Unity/Application/MirSurface.cpp−3 tests/qmltests/Stages/tst_DesktopStage.qml+1 tests/qmltests/Stages/tst_TabletStage.qml+2−5

plugins/WindowManager/TopLevelSurfaceList.cpp+82−24

@@ -21,6 +21,8 @@

2121#include <unity/shell/application/MirSurfaceInterface.h>

2222#include <unity/shell/application/MirSurfaceListInterface.h>

2323

24+#include <QMetaObject>

25+

2426Q_LOGGING_CATEGORY(UNITY_TOPSURFACELIST, "unity.topsurfacelist")

2527

2628#define DEBUG_MSG qCDebug(UNITY_TOPSURFACELIST).nospace().noquote() << __func__

@@ -69,17 +71,18 @@ void TopLevelSurfaceList::raise(MirSurfaceInterface *surface)

6971

7072 int i = indexOf(surface);

7173 if (i != -1) {

72− moveSurface(i, 0);

74+ raiseId(m_surfaceList.at(i).id);

7375 }

7476}

7577

76−void TopLevelSurfaceList::prependPlaceholder(ApplicationInfoInterface *application)

78+void TopLevelSurfaceList::appendPlaceholder(ApplicationInfoInterface *application)

7779{

7880 DEBUG_MSG << "(" << application->appId() << ")";

79− prependSurfaceHelper(nullptr, application);

81+

82+ appendSurfaceHelper(nullptr, application);

8083}

8184

82−void TopLevelSurfaceList::prependSurface(MirSurfaceInterface *surface, ApplicationInfoInterface *application)

85+void TopLevelSurfaceList::appendSurface(MirSurfaceInterface *surface, ApplicationInfoInterface *application)

8386{

8487 Q_ASSERT(surface != nullptr);

8588

@@ -89,7 +92,8 @@ void TopLevelSurfaceList::prependSurface(MirSurfaceInterface *surface, Applicati

8992 if (entry.application == application && entry.surface == nullptr) {

9093 entry.surface = surface;

9194 connectSurface(surface);

92− DEBUG_MSG << " appId=" << application->appId() << " surface=" << surface << ", filling out placeholder. after: " << toString();

95+ DEBUG_MSG << " appId=" << application->appId() << " surface=" << surface

96+ << ", filling out placeholder. after: " << toString();

9397 Q_EMIT dataChanged(index(i) /* topLeft */, index(i) /* bottomRight */, QVector<int>() << SurfaceRole);

9498 filledPlaceholder = true;

9599 }

@@ -97,22 +101,34 @@ void TopLevelSurfaceList::prependSurface(MirSurfaceInterface *surface, Applicati

97101

98102 if (!filledPlaceholder) {

99103 DEBUG_MSG << " appId=" << application->appId() << " surface=" << surface << ", adding new row";

100− prependSurfaceHelper(surface, application);

104+ appendSurfaceHelper(surface, application);

101105 }

102106}

103107

104−void TopLevelSurfaceList::prependSurfaceHelper(MirSurfaceInterface *surface, ApplicationInfoInterface *application)

108+void TopLevelSurfaceList::appendSurfaceHelper(MirSurfaceInterface *surface, ApplicationInfoInterface *application)

105109{

106− beginInsertRows(QModelIndex(), 0 /*first*/, 0 /*last*/);

110+ if (m_modelState == IdleState) {

111+ m_modelState = InsertingState;

112+ beginInsertRows(QModelIndex(), m_surfaceList.size() /*first*/, m_surfaceList.size() /*last*/);

113+ } else {

114+ Q_ASSERT(m_modelState == ResettingState);

115+ // No point in signaling anything if we're resetting the whole model

116+ }

117+

107118 int id = generateId();

108− m_surfaceList.prepend(ModelEntry(surface, application, id));

119+ m_surfaceList.append(ModelEntry(surface, application, id));

109120 if (surface) {

110121 connectSurface(surface);

111122 }

112− endInsertRows();

123+

124+ if (m_modelState == InsertingState) {

125+ endInsertRows();

126+ Q_EMIT countChanged();

127+ Q_EMIT listChanged();

128+ m_modelState = IdleState;

129+ }

130+

113131 DEBUG_MSG << " after " << toString();

114− Q_EMIT countChanged();

115− Q_EMIT listChanged();

116132}

117133

118134void TopLevelSurfaceList::connectSurface(MirSurfaceInterface *surface)

@@ -171,12 +187,24 @@ void TopLevelSurfaceList::onSurfaceDestroyed(MirSurfaceInterface *surface)

171187

172188void TopLevelSurfaceList::removeAt(int index)

173189{

174− beginRemoveRows(QModelIndex(), index, index);

190+ if (m_modelState == IdleState) {

191+ beginRemoveRows(QModelIndex(), index, index);

192+ m_modelState = RemovingState;

193+ } else {

194+ Q_ASSERT(m_modelState == ResettingState);

195+ // No point in signaling anything if we're resetting the whole model

196+ }

197+

175198 m_surfaceList.removeAt(index);

176− endRemoveRows();

199+

200+ if (m_modelState == RemovingState) {

201+ endRemoveRows();

202+ Q_EMIT countChanged();

203+ Q_EMIT listChanged();

204+ m_modelState = IdleState;

205+ }

206+

177207 DEBUG_MSG << " after " << toString();

178− Q_EMIT countChanged();

179− Q_EMIT listChanged();

180208}

181209

182210int TopLevelSurfaceList::indexOf(MirSurfaceInterface *surface)

@@ -189,7 +217,7 @@ int TopLevelSurfaceList::indexOf(MirSurfaceInterface *surface)

189217 return -1;

190218}

191219

192−void TopLevelSurfaceList::moveSurface(int from, int to)

220+void TopLevelSurfaceList::move(int from, int to)

193221{

194222 if (from == to) return;

195223 DEBUG_MSG << " from=" << from << " to=" << to;

@@ -200,11 +228,17 @@ void TopLevelSurfaceList::moveSurface(int from, int to)

200228 by one, as explained in the documentation:

201229 http://qt-project.org/doc/qt-5.0/qtcore/qabstractitemmodel.html#beginMoveRows */

202230

231+ Q_ASSERT(m_modelState == IdleState);

232+ m_modelState = MovingState;

233+

203234 beginMoveRows(parent, from, from, parent, to + (to > from ? 1 : 0));

204235 m_surfaceList.move(from, to);

205− DEBUG_MSG << " after " << toString();

206236 endMoveRows();

207237 Q_EMIT listChanged();

238+

239+ m_modelState = IdleState;

240+

241+ DEBUG_MSG << " after " << toString();

208242 }

209243}

210244

@@ -245,12 +279,27 @@ int TopLevelSurfaceList::indexForId(int id) const

245279 return -1;

246280}

247281

248−void TopLevelSurfaceList::move(int id, int toIndex)

282+void TopLevelSurfaceList::doRaiseId(int id)

249283{

250− DEBUG_MSG << " id=" << id << " toIndex=" << toIndex;

251284 int fromIndex = indexForId(id);

252285 if (fromIndex != -1) {

253− moveSurface(fromIndex, toIndex);

286+ move(fromIndex, 0 /* toIndex */);

287+ }

288+}

289+

290+void TopLevelSurfaceList::raiseId(int id)

291+{

292+ if (m_modelState == IdleState) {

293+ DEBUG_MSG << "(id=" << id << ") - do it now.";

294+ doRaiseId(id);

295+ } else {

296+ DEBUG_MSG << "(id=" << id << ") - Model busy (modelState=" << m_modelState << "). Try again in the next event loop.";

297+ // The model has just signalled some change. If we have a Repeater responding to this update, it will get nuts

298+ // if we perform yet another model change straight away.

299+ //

300+ // A bad sympton of this problem is a Repeater.itemAt(index) call returning null event though Repeater.count says

301+ // the index is definitely within bounds.

302+ QMetaObject::invokeMethod(this, "raiseId", Qt::QueuedConnection, Q_ARG(int, id));

254303 }

255304}

256305

@@ -306,10 +355,10 @@ void TopLevelSurfaceList::addApplication(ApplicationInfoInterface *application)

306355

307356 if (application->state() != ApplicationInfoInterface::Stopped) {

308357 if (surfaceList->count() == 0) {

309− prependPlaceholder(application);

358+ appendPlaceholder(application);

310359 } else {

311360 for (int i = 0; i < surfaceList->count(); ++i) {

312− prependSurface(surfaceList->get(i), application);

361+ appendSurface(surfaceList->get(i), application);

313362 }

314363 }

315364 }

@@ -318,7 +367,7 @@ void TopLevelSurfaceList::addApplication(ApplicationInfoInterface *application)

318367 [this, application, surfaceList](const QModelIndex & /*parent*/, int first, int last)

319368 {

320369 for (int i = last; i >= first; --i) {

321− this->prependSurface(surfaceList->get(i), application);

370+ this->appendSurface(surfaceList->get(i), application);

322371 }

323372 });

324373}

@@ -332,6 +381,9 @@ void TopLevelSurfaceList::removeApplication(ApplicationInfoInterface *applicatio

332381

333382 disconnect(surfaceList, 0, this, 0);

334383

384+ Q_ASSERT(m_modelState == IdleState);

385+ m_modelState = RemovingState;

386+

335387 int i = 0;

336388 while (i < m_surfaceList.count()) {

337389 if (m_surfaceList.at(i).application == application) {

@@ -345,6 +397,8 @@ void TopLevelSurfaceList::removeApplication(ApplicationInfoInterface *applicatio

345397 }

346398 }

347399

400+ m_modelState = IdleState;

401+

348402 DEBUG_MSG << " after " << toString();

349403

350404 m_applications.removeAll(application);

@@ -363,6 +417,9 @@ void TopLevelSurfaceList::setApplicationsModel(QAbstractListModel* value)

363417

364418 DEBUG_MSG << "(" << value << ")";

365419

420+ Q_ASSERT(m_modelState == IdleState);

421+ m_modelState = ResettingState;

422+

366423 beginResetModel();

367424

368425 if (m_applicationsModel) {

@@ -399,6 +456,7 @@ void TopLevelSurfaceList::setApplicationsModel(QAbstractListModel* value)

399456 }

400457

401458 endResetModel();

459+ m_modelState = IdleState;

402460}

403461

404462ApplicationInfoInterface *TopLevelSurfaceList::getApplicationFromModelAt(int index)

plugins/WindowManager/TopLevelSurfaceList.h+19−6

@@ -124,13 +124,17 @@ public Q_SLOTS:

124124

125125 /**

126126 * @brief Returns the index where the row with the given id is located

127+ *

128+ * Returns -1 if there's no row with the given id.

127129 */

128130 int indexForId(int id) const;

129131

130132 /**

131− * @brief Moves the row with the given id to the specified index

133+ * @brief Raises the row with the given id to index 0

132134 */

133− void move(int id, int toIndex);

135+ void raiseId(int id);

136+

137+ void doRaiseId(int id);

134138

135139Q_SIGNALS:

136140 void countChanged();

@@ -152,8 +156,8 @@ private:

152156

153157 int indexOf(unity::shell::application::MirSurfaceInterface *surface);

154158 void raise(unity::shell::application::MirSurfaceInterface *surface);

155− void moveSurface(int from, int to);

156− void prependSurfaceHelper(unity::shell::application::MirSurfaceInterface *surface,

159+ void move(int from, int to);

160+ void appendSurfaceHelper(unity::shell::application::MirSurfaceInterface *surface,

157161 unity::shell::application::ApplicationInfoInterface *application);

158162 void connectSurface(unity::shell::application::MirSurfaceInterface *surface);

159163 int generateId();

@@ -173,7 +177,7 @@ private:

173177

174178 It's a model row containing a null surface and the given application.

175179 */

176− void prependPlaceholder(unity::shell::application::ApplicationInfoInterface *application);

180+ void appendPlaceholder(unity::shell::application::ApplicationInfoInterface *application);

177181

178182 /*

179183 Adds a model row with the given surface and application

@@ -181,7 +185,7 @@ private:

181185 Alternatively, if a placeholder exists for the given application it's

182186 filled with the given surface instead.

183187 */

184− void prependSurface(unity::shell::application::MirSurfaceInterface *surface,

188+ void appendSurface(unity::shell::application::MirSurfaceInterface *surface,

185189 unity::shell::application::ApplicationInfoInterface *application);

186190

187191 struct ModelEntry {

@@ -202,6 +206,15 @@ private:

202206

203207 QAbstractListModel* m_applicationsModel{nullptr};

204208 int m_applicationRole{-1};

209+

210+ enum ModelState {

211+ IdleState,

212+ InsertingState,

213+ RemovingState,

214+ MovingState,

215+ ResettingState

216+ };

217+ ModelState m_modelState{IdleState};

205218};

206219

207220Q_DECLARE_METATYPE(TopLevelSurfaceList*)

qml/Shell.qml+4

@@ -265,6 +265,10 @@ Item {

265265 return "Stages/DesktopStage.qml";

266266 }

267267 }

268+ // TODO: Ensure the current stage is destroyed before the new one gets loaded.

269+ // Currently the new one will get loaded while the old is still hanging

270+ // around for a bit, which might lead to conflicts where both stages

271+ // change the model simultaneously.

268272 onQmlComponentChanged: {

269273 if (item) item.stageAboutToBeUnloaded();

270274 source = qmlComponent;

qml/Stages/DesktopStage.qml+13−6

@@ -215,6 +215,7 @@ AbstractStage {

215215 target: MirFocusController

216216 property: "focusedSurface"

217217 value: priv.focusedAppDelegate ? priv.focusedAppDelegate.surface : null

218+ when: !appRepeater.startingUp && root.parent

218219 }

219220

220221 FocusScope {

@@ -231,7 +232,7 @@ AbstractStage {

231232 fillMode: Image.PreserveAspectCrop

232233 }

233234

234− Repeater {

235+ TopLevelSurfaceRepeater {

235236 id: appRepeater

236237 model: topLevelSurfaceList

237238 objectName: "appRepeater"

@@ -294,19 +295,25 @@ AbstractStage {

294295 }

295296

296297 onFocusChanged: {

297− if (focus) {

298+ if (focus && !appRepeater.startingUp) {

298299 priv.focusedAppDelegate = appDelegate;

299− topLevelSurfaceList.move(model.id, 0);

300300 }

301301 }

302302 Component.onCompleted: {

303− // a top level window is always the focused one when it first appears, unfocusing

304− // any preexisting one

305− if (index == 0) {

303+ // NB: We're differentiating if this delegate was created in response to a new entry in the model

304+ // or if the Repeater is just populating itself with delegates to match the model it received.

305+ if (!appRepeater.startingUp) {

306+ // a top level window is always the focused one when it first appears, unfocusing

307+ // any preexisting one

306308 focus = true;

307309 }

308310 }

309311 Component.onDestruction: {

312+ if (!root.parent) {

313+ // This stage is about to be destroyed. Don't mess up with the model at this point

314+ return;

315+ }

316+

310317 if (focus) {

311318 // focus some other window

312319 for (var i = 0; i < appRepeater.count; i++) {

qml/Stages/PhoneStage.qml+17−6

@@ -227,6 +227,7 @@ AbstractStage {

227227 target: MirFocusController

228228 property: "focusedSurface"

229229 value: priv.focusedAppDelegate ? priv.focusedAppDelegate.surface : null

230+ when: root.parent && !spreadRepeater.startingUp

230231 }

231232

232233 Flickable {

@@ -446,7 +447,7 @@ AbstractStage {

446447 }

447448 }

448449

449− Repeater {

450+ TopLevelSurfaceRepeater {

450451 id: spreadRepeater

451452 objectName: "spreadRepeater"

452453 model: topLevelSurfaceList

@@ -497,17 +498,27 @@ AbstractStage {

497498 readonly property bool isDash: model.application.appId == "unity8-dash"

498499

499500 Component.onCompleted: {

500− // a top level window is always the focused one when it first appears, unfocusing

501− // any preexisting one

502− if (model.index == 0) {

501+ // NB: We're differentiating if this delegate was created in response to a new entry in the model

502+ // or if the Repeater is just populating itself with delegates to match the model it received.

503+ if (!spreadRepeater.startingUp) {

504+ // a top level window is always the focused one when it first appears, unfocusing

505+ // any preexisting one

506+ //

507+ // new items are appended and must be manually brought to front.

508+ // that's how it *must* be in order to get the animation for new

509+ // surfaces working

503510 focus = true;

504511 }

505512 }

506513

507514 onFocusChanged: {

508− if (focus) {

515+ if (focus && !spreadRepeater.startingUp) {

509516 priv.focusedAppDelegate = appDelegate;

510− topLevelSurfaceList.move(model.id, 0);

517+ // If we're orphan (!parent) it means this stage is no longer the current one

518+ // and will be deleted shortly. So we should no longer have a say over the model

519+ if (root.parent) {

520+ topLevelSurfaceList.raiseId(model.id);

521+ }

511522 }

512523 }

513524 Connections {

qml/Stages/TabletStage.qml+8−12

@@ -167,11 +167,6 @@ AbstractStage {

167167 onListChanged: priv.updateMainAndSideStageIndexes()

168168 }

169169

170− onTopLevelSurfaceListChanged: {

171− // Initialize their values

172− priv.updateMainAndSideStageIndexes();

173− }

174−

175170 QtObject {

176171 id: priv

177172 objectName: "stagesPriv"

@@ -183,7 +178,7 @@ AbstractStage {

183178 if (!root.topLevelSurfaceList)

184179 return;

185180

186− for (var i = 0; i < root.topLevelSurfaceList.count && (!choseMainStage || !choseSideStage); ++i) {

181+ for (var i = 0; i < spreadRepeater.count && (!choseMainStage || !choseSideStage); ++i) {

187182 var spreadDelegate = spreadRepeater.itemAt(i);

188183 if (!spreadView.sideStageHidden && spreadDelegate.stage == ApplicationInfoInterface.SideStage

189184 && !choseSideStage) {

@@ -298,6 +293,7 @@ AbstractStage {

298293 target: MirFocusController

299294 property: "focusedSurface"

300295 value: priv.focusedAppDelegate ? priv.focusedAppDelegate.surface : null

296+ when: root.parent && !spreadRepeater.startingUp

301297 }

302298

303299 Flickable {

@@ -687,18 +683,20 @@ AbstractStage {

687683 }

688684 }

689685

690− Repeater {

686+ TopLevelSurfaceRepeater {

691687 id: spreadRepeater

692688 objectName: "spreadRepeater"

693689 model: root.topLevelSurfaceList

694690

695691 onItemAdded: {

692+ priv.updateMainAndSideStageIndexes();

696693 if (spreadView.phase == 2) {

697694 spreadView.snapTo(index);

698695 }

699696 }

700697

701698 onItemRemoved: {

699+ priv.updateMainAndSideStageIndexes();

702700 // Unless we're closing the app ourselves in the spread,

703701 // lets make sure the spread doesn't mess up by the changing app list.

704702 if (spreadView.closingIndex == -1) {

@@ -746,9 +744,9 @@ AbstractStage {

746744 readonly property bool isDash: model.application.appId == "unity8-dash"

747745

748746 onFocusChanged: {

749− if (focus) {

747+ if (focus && !spreadRepeater.startingUp) {

750748 priv.focusedAppDelegate = spreadTile;

751− root.topLevelSurfaceList.move(model.id, 0);

749+ root.topLevelSurfaceList.raiseId(model.id);

752750 }

753751 if (focus && priv.sideStageEnabled && stage === ApplicationInfoInterface.SideStage) {

754752 sideStage.show();

@@ -824,9 +822,7 @@ AbstractStage {

824822 Component.onCompleted: {

825823 // a top level window is always the focused one when it first appears, unfocusing

826824 // any preexisting one

827− if (model.index == 0) {

828− focus = true;

829− }

825+ focus = true;

830826 refreshStage();

831827 _constructing = false;

832828 }

qml/Stages/TopLevelSurfaceRepeater.qmlnew file+52

@@ -0,0 +1,52 @@

1+/*

3+ *

4+ * This program is free software; you can redistribute it and/or modify

5+ * it under the terms of the GNU General Public License as published by

6+ * the Free Software Foundation; version 3.

7+ *

8+ * This program is distributed in the hope that it will be useful,

9+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

10+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

11+ * GNU General Public License for more details.

12+ *

13+ * You should have received a copy of the GNU General Public License

14+ * along with this program. If not, see <http://www.gnu.org/licenses/>.

15+ */

16+

17+import QtQuick 2.4

18+

19+Repeater {

20+ id: root

21+ // FIXME: This is a hack around us not knowing whether the Repeater has finished creating its

22+ // delegates on start up.

23+ // This is a problem when the stage gets a TopLevelSurfaceList already populated with several

24+ // rows.

25+ property bool startingUp: true

26+ onStartingUpChanged: {

27+ if (!startingUp) {

28+ // the top-most surface must be the focused one.

29+ var topmostDelegate = itemAt(0);

30+ topmostDelegate.focus = true;

31+ }

32+ }

33+

34+ onItemAdded: {

35+ if (startingUp) {

36+ checkIfStillStartingUp();

37+ }

38+ }

39+

40+ function checkIfStillStartingUp() {

41+ var i = 0;

42+ var missingDelegate = false;

43+ for (i = 0; i < model.count && !missingDelegate; ++i) {

44+ if (!itemAt(i)) {

45+ missingDelegate = true;

46+ }

47+ }

48+ if (!missingDelegate) {

49+ startingUp = false;

50+ }

51+ }

52+}

tests/mocks/Unity/Application/MirSurface.cpp−3

@@ -263,9 +263,6 @@ void MirSurface::setActiveFocus(bool value)

263263 return;

264264

265265 m_activeFocus = value;

266− if (value) {

267− MirFocusController::instance()->setFocusedSurface(this);

268− }

269266

270267 Q_EMIT activeFocusChanged(value);

271268}

tests/qmltests/Stages/tst_DesktopStage.qml+1

@@ -20,6 +20,7 @@ import Ubuntu.Components 1.3

2020import Ubuntu.Components.ListItems 1.3

2121import Unity.Application 0.1

2222import Unity.Test 0.1

23+import WindowManager 0.1

2324import Utils 0.1

2425

2526import ".." // For EdgeBarrierControls

tests/qmltests/Stages/tst_TabletStage.qml+2−5

@@ -21,6 +21,7 @@ import Ubuntu.Components.ListItems 1.3 as ListItem

2121import Unity.Application 0.1

2222import Unity.Test 0.1

2323import Utils 0.1

24+import WindowManager 0.1

2425

2526import ".."

2627import "../../../qml/Stages"

@@ -170,11 +171,7 @@ Rectangle {

170171 // Shell instance gets destroyed.

171172 tryCompare(tabletStageLoader, "itemDestroyed", true);

172173

173− // kill all (fake) running apps

174− webbrowserCheckBox.checked = false;

175− galleryCheckBox.checked = false;

176− dialerCheckBox.checked = false;

177− facebookCheckBox.checked = false;

174+ killApps();

178175 WindowStateStorage.clear();

179176 }

180177