Changeset View
Standalone View
tabgroup.cpp
Show All 21 Lines | |||||
22 | 22 | | |||
23 | #include "client.h" | 23 | #include "client.h" | ||
24 | #include "effects.h" | 24 | #include "effects.h" | ||
25 | #include "workspace.h" | 25 | #include "workspace.h" | ||
26 | 26 | | |||
27 | namespace KWin | 27 | namespace KWin | ||
28 | { | 28 | { | ||
29 | 29 | | |||
30 | TabGroup::TabGroup(Client *c) | 30 | TabGroup::TabGroup(AbstractClient *c) | ||
31 | : m_clients() | 31 | : m_clients() | ||
32 | , m_current(c) | 32 | , m_current(c) | ||
33 | , m_minSize(c->minSize()) | 33 | , m_minSize(c->minSize()) | ||
34 | , m_maxSize(c->maxSize()) | 34 | , m_maxSize(c->maxSize()) | ||
35 | , m_stateUpdatesBlocked(0) | 35 | , m_stateUpdatesBlocked(0) | ||
36 | , m_pendingUpdates(TabGroup::None) | 36 | , m_pendingUpdates(TabGroup::None) | ||
37 | { | 37 | { | ||
38 | QIcon icon(c->icon()); | 38 | QIcon icon(c->icon()); | ||
Show All 14 Lines | |||||
53 | } | 53 | } | ||
54 | 54 | | |||
55 | void TabGroup::activatePrev() | 55 | void TabGroup::activatePrev() | ||
56 | { | 56 | { | ||
57 | int index = m_clients.indexOf(m_current); | 57 | int index = m_clients.indexOf(m_current); | ||
58 | setCurrent(m_clients.at((index > 0) ? index - 1 : m_clients.count() - 1)); | 58 | setCurrent(m_clients.at((index > 0) ? index - 1 : m_clients.count() - 1)); | ||
59 | } | 59 | } | ||
60 | 60 | | |||
61 | bool TabGroup::add(Client* c, Client *other, bool after, bool becomeVisible) | 61 | bool TabGroup::add(AbstractClient* c, AbstractClient *other, bool after, bool becomeVisible) | ||
62 | { | 62 | { | ||
63 | Q_ASSERT(!c->tabGroup()); | 63 | Q_ASSERT(!c->tabGroup()); | ||
64 | 64 | | |||
65 | if (contains(c) || !contains(other)) | 65 | if (contains(c) || !contains(other)) | ||
66 | return false; | 66 | return false; | ||
67 | 67 | | |||
68 | // Tabbed windows MUST have a decoration | 68 | // Tabbed windows MUST have a decoration | ||
69 | c->setNoBorder(false); | 69 | c->setNoBorder(false); | ||
▲ Show 20 Lines • Show All 69 Lines • ▼ Show 20 Line(s) | 136 | else { | |||
139 | m_current = c; // setCurrent will be called by Toplevel::setReadyForPainting() | 139 | m_current = c; // setCurrent will be called by Toplevel::setReadyForPainting() | ||
140 | } | 140 | } | ||
141 | } | 141 | } | ||
142 | 142 | | |||
143 | m_current->triggerDecorationRepaint(); | 143 | m_current->triggerDecorationRepaint(); | ||
144 | return true; | 144 | return true; | ||
145 | } | 145 | } | ||
146 | 146 | | |||
147 | bool TabGroup::remove(Client* c) | 147 | bool TabGroup::remove(AbstractClient* c) | ||
148 | { | 148 | { | ||
149 | if (!c) | 149 | if (!c) | ||
150 | return false; | 150 | return false; | ||
151 | 151 | | |||
152 | int index = m_clients.indexOf(c); | 152 | int index = m_clients.indexOf(c); | ||
153 | if (index < 0) | 153 | if (index < 0) | ||
154 | return false; | 154 | return false; | ||
155 | 155 | | |||
Show All 28 Lines | |||||
184 | 184 | | |||
185 | void TabGroup::closeAll() | 185 | void TabGroup::closeAll() | ||
186 | { | 186 | { | ||
187 | // NOTICE - in theory it's OK to use the list because closing sends an event to the client and | 187 | // NOTICE - in theory it's OK to use the list because closing sends an event to the client and | ||
188 | // due to the async X11 nature, the client would be released and thus removed from m_clients | 188 | // due to the async X11 nature, the client would be released and thus removed from m_clients | ||
189 | // after this function exits. | 189 | // after this function exits. | ||
190 | // However later Wayland support or similar might not share this bahaviour - and we really had | 190 | // However later Wayland support or similar might not share this bahaviour - and we really had | ||
191 | // enough trouble with a polluted client list around the tabbing code .... | 191 | // enough trouble with a polluted client list around the tabbing code .... | ||
192 | ClientList list(m_clients); | 192 | auto list(m_clients); | ||
193 | for (ClientList::const_iterator i = list.constBegin(), end = list.constEnd(); i != end; ++i) | 193 | for (auto i = list.constBegin(), end = list.constEnd(); i != end; ++i) | ||
romangg: fix coding style: brackets | |||||
zzag: Why not range-based for loop? | |||||
answer for both comments: I tried to do a least invasive refactoring. Thus only the change from ClientList to whatever the current type is. I did not want to change the logic of the loop by using range-based for and also did not want to fix the coding style. For new code it would of course be a range based for and {} graesslin: answer for both comments: I tried to do a least invasive refactoring. Thus only the change from… | |||||
194 | if (*i != m_current) | 194 | if (*i != m_current) | ||
195 | (*i)->closeWindow(); | 195 | (*i)->closeWindow(); | ||
196 | 196 | | |||
197 | m_current->closeWindow(); | 197 | m_current->closeWindow(); | ||
198 | } | 198 | } | ||
199 | 199 | | |||
200 | void TabGroup::move(Client *c, Client *other, bool after) | 200 | void TabGroup::move(AbstractClient *c, AbstractClient *other, bool after) | ||
201 | { | 201 | { | ||
202 | if (c == other) | 202 | if (c == other) | ||
203 | return; | 203 | return; | ||
204 | 204 | | |||
205 | int from = m_clients.indexOf(c); | 205 | int from = m_clients.indexOf(c); | ||
206 | if (from < 0) | 206 | if (from < 0) | ||
207 | return; | 207 | return; | ||
208 | 208 | | |||
209 | int to = other ? m_clients.indexOf(other) : m_clients.size() - 1; | 209 | int to = other ? m_clients.indexOf(other) : m_clients.size() - 1; | ||
210 | if (to < 0) | 210 | if (to < 0) | ||
211 | return; | 211 | return; | ||
212 | to += after; | 212 | to += after; | ||
213 | if (to >= m_clients.size()) | 213 | if (to >= m_clients.size()) | ||
214 | to = m_clients.size() - 1; | 214 | to = m_clients.size() - 1; | ||
215 | 215 | | |||
216 | if (from == to) | 216 | if (from == to) | ||
217 | return; | 217 | return; | ||
218 | 218 | | |||
219 | m_clients.move(from, to); | 219 | m_clients.move(from, to); | ||
220 | m_current->triggerDecorationRepaint(); | 220 | m_current->triggerDecorationRepaint(); | ||
221 | } | 221 | } | ||
222 | 222 | | |||
223 | bool TabGroup::isActive() const | 223 | bool TabGroup::isActive() const | ||
224 | { | 224 | { | ||
225 | return contains(dynamic_cast<Client*>(Workspace::self()->activeClient())); | 225 | return contains(Workspace::self()->activeClient()); | ||
226 | } | 226 | } | ||
227 | 227 | | |||
228 | void TabGroup::setCurrent(Client* c, bool force) | 228 | void TabGroup::setCurrent(AbstractClient* c, bool force) | ||
229 | { | 229 | { | ||
230 | if ((c == m_current && !force) || !contains(c)) | 230 | if ((c == m_current && !force) || !contains(c)) | ||
231 | return; | 231 | return; | ||
232 | 232 | | |||
233 | // Notify effects of switch | 233 | // Notify effects of switch | ||
234 | if (effects) | 234 | if (effects) | ||
235 | static_cast<EffectsHandlerImpl*>(effects)->slotCurrentTabAboutToChange(m_current->effectWindow(), c->effectWindow()); | 235 | static_cast<EffectsHandlerImpl*>(effects)->slotCurrentTabAboutToChange(m_current->effectWindow(), c->effectWindow()); | ||
236 | 236 | | |||
237 | m_current = c; | 237 | m_current = c; | ||
238 | c->setClientShown(true); // reduce flicker? | 238 | c->setClientShown(true); // reduce flicker? | ||
239 | for (ClientList::const_iterator i = m_clients.constBegin(), end = m_clients.constEnd(); i != end; ++i) | 239 | for (auto i = m_clients.constBegin(), end = m_clients.constEnd(); i != end; ++i) | ||
240 | (*i)->setClientShown((*i) == m_current); | 240 | (*i)->setClientShown((*i) == m_current); | ||
241 | } | 241 | } | ||
242 | 242 | | |||
243 | void TabGroup::sync(const char *property, Client *c) | 243 | void TabGroup::sync(const char *property, AbstractClient *c) | ||
244 | { | 244 | { | ||
245 | if (c->metaObject()->indexOfProperty(property) > -1) { | 245 | if (c->metaObject()->indexOfProperty(property) > -1) { | ||
246 | qCWarning(KWIN_CORE, "caught attempt to sync non dynamic property: %s", property); | 246 | qCWarning(KWIN_CORE, "caught attempt to sync non dynamic property: %s", property); | ||
247 | return; | 247 | return; | ||
248 | } | 248 | } | ||
249 | QVariant v = c->property(property); | 249 | QVariant v = c->property(property); | ||
250 | for (ClientList::iterator i = m_clients.begin(), end = m_clients.end(); i != end; ++i) { | 250 | for (auto i = m_clients.begin(), end = m_clients.end(); i != end; ++i) { | ||
251 | if (*i != m_current) | 251 | if (*i != m_current) | ||
252 | (*i)->setProperty(property, v); | 252 | (*i)->setProperty(property, v); | ||
253 | } | 253 | } | ||
254 | } | 254 | } | ||
255 | 255 | | |||
256 | void TabGroup::updateMinMaxSize() | 256 | void TabGroup::updateMinMaxSize() | ||
257 | { | 257 | { | ||
258 | // Determine entire group's minimum and maximum sizes | 258 | // Determine entire group's minimum and maximum sizes | ||
259 | // TODO this used to be signalled out but i didn't find a receiver - or got an idea what this would be good for | 259 | // TODO this used to be signalled out but i didn't find a receiver - or got an idea what this would be good for | ||
260 | // find purpose & solution or kick it | 260 | // find purpose & solution or kick it | ||
261 | // QSize oldMin = m_minSize; | 261 | // QSize oldMin = m_minSize; | ||
262 | // QSize oldMax = m_maxSize; | 262 | // QSize oldMax = m_maxSize; | ||
263 | m_minSize = QSize(0, 0); | 263 | m_minSize = QSize(0, 0); | ||
264 | m_maxSize = QSize(INT_MAX, INT_MAX); | 264 | m_maxSize = QSize(INT_MAX, INT_MAX); | ||
265 | 265 | | |||
266 | for (ClientList::const_iterator i = m_clients.constBegin(); i != m_clients.constEnd(); ++i) { | 266 | for (auto i = m_clients.constBegin(); i != m_clients.constEnd(); ++i) { | ||
267 | m_minSize = m_minSize.expandedTo((*i)->minSize()); | 267 | m_minSize = m_minSize.expandedTo((*i)->minSize()); | ||
268 | m_maxSize = m_maxSize.boundedTo((*i)->maxSize()); | 268 | m_maxSize = m_maxSize.boundedTo((*i)->maxSize()); | ||
269 | } | 269 | } | ||
270 | 270 | | |||
271 | // TODO: this actually resolves a conflict that should be caught when adding? | 271 | // TODO: this actually resolves a conflict that should be caught when adding? | ||
272 | m_maxSize = m_maxSize.expandedTo(m_minSize); | 272 | m_maxSize = m_maxSize.expandedTo(m_minSize); | ||
273 | 273 | | |||
274 | // calculate this _once_ to get a common size. | 274 | // calculate this _once_ to get a common size. | ||
275 | // TODO this leaves another unresolved conflict about the base increment (luckily not used too often) | 275 | // TODO this leaves another unresolved conflict about the base increment (luckily not used too often) | ||
276 | const QSize size = m_current->clientSize().expandedTo(m_minSize).boundedTo(m_maxSize); | 276 | const QSize size = m_current->clientSize().expandedTo(m_minSize).boundedTo(m_maxSize); | ||
277 | if (size != m_current->clientSize()) { | 277 | if (size != m_current->clientSize()) { | ||
278 | const QRect r(m_current->pos(), m_current->sizeForClientSize(size)); | 278 | const QRect r(m_current->pos(), m_current->sizeForClientSize(size)); | ||
279 | for (ClientList::const_iterator i = m_clients.constBegin(), end = m_clients.constEnd(); i != end; ++i) | 279 | for (auto i = m_clients.constBegin(), end = m_clients.constEnd(); i != end; ++i) | ||
280 | (*i)->setGeometry(r); | 280 | (*i)->setGeometry(r); | ||
281 | } | 281 | } | ||
282 | } | 282 | } | ||
283 | 283 | | |||
284 | 284 | | |||
285 | void TabGroup::blockStateUpdates(bool more) { | 285 | void TabGroup::blockStateUpdates(bool more) { | ||
286 | more ? ++m_stateUpdatesBlocked : --m_stateUpdatesBlocked; | 286 | more ? ++m_stateUpdatesBlocked : --m_stateUpdatesBlocked; | ||
287 | if (m_stateUpdatesBlocked < 0) { | 287 | if (m_stateUpdatesBlocked < 0) { | ||
288 | m_stateUpdatesBlocked = 0; | 288 | m_stateUpdatesBlocked = 0; | ||
289 | qCWarning(KWIN_CORE, "TabGroup: Something is messed up with TabGroup::blockStateUpdates() invocation\nReleased more than blocked!"); | 289 | qCWarning(KWIN_CORE, "TabGroup: Something is messed up with TabGroup::blockStateUpdates() invocation\nReleased more than blocked!"); | ||
290 | } | 290 | } | ||
291 | } | 291 | } | ||
292 | 292 | | |||
293 | void TabGroup::updateStates(Client* main, States states, Client* only) | 293 | void TabGroup::updateStates(AbstractClient* main, States states, AbstractClient* only) | ||
294 | { | 294 | { | ||
295 | if (main == only) | 295 | if (main == only) | ||
296 | return; // there's no need to only align "us" to "us" | 296 | return; // there's no need to only align "us" to "us" | ||
297 | if (m_stateUpdatesBlocked > 0) { | 297 | if (m_stateUpdatesBlocked > 0) { | ||
298 | m_pendingUpdates |= states; | 298 | m_pendingUpdates |= states; | ||
299 | return; | 299 | return; | ||
300 | } | 300 | } | ||
301 | 301 | | |||
302 | states |= m_pendingUpdates; | 302 | states |= m_pendingUpdates; | ||
303 | m_pendingUpdates = TabGroup::None; | 303 | m_pendingUpdates = TabGroup::None; | ||
304 | 304 | | |||
305 | ClientList toBeRemoved, onlyDummy; | 305 | decltype(m_clients) toBeRemoved, onlyDummy; | ||
The keyword decltype was new to me and so I read up on it. This took multiple hours because it has quite complex semantics depending on the expression it evaluates. Still I'm happy I had to do it, so I learned about Rvalue references and so on in modern C++. Still (and although it should be the simple case here by evaluating a simple expression, which gives just the type of the variable back) I would refrain from using it here and name the type explicitly instead. To be more general from what I've read on it I would prefer to use decltype only when it provides a large benefit, i.e. in certain templated library functions, and not just to replace explicitly writing out a type in simple sequential code. In the end it's your decision but I don't see how the usage of decltype simplifies the code here. I think it makes it more difficult to understand, most certainly for potential contributors, who know basic C++, but not the advanced tools for library writers introduced in modern C++. romangg: The keyword `decltype` was new to me and so I read up on it. This took multiple hours because… | |||||
I'll change it. I used decltype as I expected the type to change during the refactoring and just forgot to switch to a proper type. graesslin: I'll change it. I used decltype as I expected the type to change during the refactoring and… | |||||
306 | ClientList *list = &m_clients; | 306 | auto *list = &m_clients; | ||
307 | if (only) { | 307 | if (only) { | ||
308 | onlyDummy << only; | 308 | onlyDummy << only; | ||
309 | list = &onlyDummy; | 309 | list = &onlyDummy; | ||
310 | } | 310 | } | ||
311 | 311 | | |||
312 | for (ClientList::const_iterator i = list->constBegin(), end = list->constEnd(); i != end; ++i) { | 312 | for (auto i = list->constBegin(), end = list->constEnd(); i != end; ++i) { | ||
313 | Client *c = (*i); | 313 | auto *c = (*i); | ||
zzag: IMHO, auto should not be used here because the type of c is not obvious. | |||||
According to Scott Meyer's Effictive modern C++ one should "Prefer auto to explicit type declarations". I think it's totally obvious what the type is when using a good IDE like kdevelop. IMHO there is no other way to read code than using KDevelop. graesslin: According to Scott Meyer's Effictive modern C++ one should "Prefer auto to explicit type… | |||||
Not all people use IDEs. Also, if a piece of code is readable only in IDEs, maybe, that's a sign that something is wrong. Sure, refactoring is much easier with auto, but it's harder to read code. IMHO, auto should be used wisely. E.g. when I call list.begin(), I know that the return type of this call is some kind of iterator so I can use auto, the code is still readable. But when I do auto* foo = (*i);, the type of foo is not really clear, the code is not readable at all. zzag: Not all people use IDEs. Also, if a piece of code is readable only in IDEs, maybe, that's a… | |||||
314 | if (c != main) { | 314 | if (c != main) { | ||
315 | if ((states & Minimized) && c->isMinimized() != main->isMinimized()) { | 315 | if ((states & Minimized) && c->isMinimized() != main->isMinimized()) { | ||
316 | if (main->isMinimized()) | 316 | if (main->isMinimized()) | ||
317 | c->minimize(true); | 317 | c->minimize(true); | ||
318 | else | 318 | else | ||
319 | c->unminimize(true); | 319 | c->unminimize(true); | ||
320 | } | 320 | } | ||
321 | 321 | | |||
Show All 26 Lines | |||||
348 | 348 | | |||
349 | // If it's not possible to have the same states then ungroup them, TODO: Check all states | 349 | // If it's not possible to have the same states then ungroup them, TODO: Check all states | ||
350 | if (((states & Geometry) && c->geometry() != main->geometry()) || | 350 | if (((states & Geometry) && c->geometry() != main->geometry()) || | ||
351 | ((states & Desktop) && c->desktop() != main->desktop())) | 351 | ((states & Desktop) && c->desktop() != main->desktop())) | ||
352 | toBeRemoved << c; | 352 | toBeRemoved << c; | ||
353 | } | 353 | } | ||
354 | } | 354 | } | ||
355 | 355 | | |||
356 | for (ClientList::const_iterator i = toBeRemoved.constBegin(), end = toBeRemoved.constEnd(); i != end; ++i) | 356 | for (auto i = toBeRemoved.constBegin(), end = toBeRemoved.constEnd(); i != end; ++i) | ||
357 | remove(*i); | 357 | remove(*i); | ||
358 | } | 358 | } | ||
359 | 359 | | |||
360 | } | 360 | } |
fix coding style: brackets