Changeset View
Standalone View
useractions.cpp
Show First 20 Lines • Show All 1562 Lines • ▼ Show 20 Line(s) | |||||
1563 | /*! | 1563 | /*! | ||
1564 | Switches to the nearest window in given direction | 1564 | Switches to the nearest window in given direction | ||
1565 | */ | 1565 | */ | ||
1566 | void Workspace::switchWindow(Direction direction) | 1566 | void Workspace::switchWindow(Direction direction) | ||
1567 | { | 1567 | { | ||
1568 | if (!active_client) | 1568 | if (!active_client) | ||
1569 | return; | 1569 | return; | ||
1570 | AbstractClient *c = active_client; | 1570 | AbstractClient *c = active_client; | ||
1571 | Client *switchTo = 0; | 1571 | int desktopNumber = c->isOnAllDesktops() ? VirtualDesktopManager::self()->current() : c->desktop(); | ||
1572 | int bestScore = 0; | 1572 | | ||
1573 | int d = c->isOnAllDesktops() ? VirtualDesktopManager::self()->current() : c->desktop(); | | |||
1574 | // Centre of the active window | 1573 | // Centre of the active window | ||
1575 | QPoint curPos(c->pos().x() + c->geometry().width() / 2, | 1574 | QPoint curPos(c->pos().x() + c->geometry().width() / 2, | ||
1576 | c->pos().y() + c->geometry().height() / 2); | 1575 | c->pos().y() + c->geometry().height() / 2); | ||
1577 | 1576 | | |||
graesslin: given that it's only called from one method I think this should be turned into a lambda or… | |||||
In terms of code (I'm new in C++), to turn this method in static local method, I must append a static keywork in the method declaration in workspace.h or there is something else I must do? Regarding to turning it into a lambda, are you suggesting to inline the code using lambda syntax or to create a variable to hold its body? Given that this method is non-trivial, I would find it harder to understand its purpose if it was somehow anonymous. pedroarthurp: In terms of code (I'm new in C++), to turn this method in static local method, I must append a… | |||||
a "static local" is a function w/o a declaration in the header (it's not part of any class) and a "static" keyword. The latter hides its visibility, so you'd simply put static QPoint oppositePosition(.) above the usage (that's important) An anonymous closure makes no sense at all (But you could really unroll the function, ie. move the code to where it's called. The function is absolutely trivial and flips the direction In doubt a comment should do. luebking: a "static local" is a function w/o a declaration in the header (it's not part of any class) and… | |||||
Got it for the static function, but I run into the following errors while trying to follow your tip: /home/pedroarthur/repositories/kde/kwin/useractions.cpp:1557:46: error: ‘Direction’ has not been declared static QPoint oppositePosition(QPoint point, Direction direction) ^~~~~~~~~ Among others. If I put workspace::Direction in the direction parameter, I get: /home/pedroarthur/repositories/kde/kwin/useractions.cpp: In function ‘QPoint KWin::oppositePosition(QPoint, KWin::Workspace::Direction)’: /home/pedroarthur/repositories/kde/kwin/useractions.cpp:1557:57: error: ‘enum KWin::Workspace::Direction’ is private within this context static QPoint oppositePosition(QPoint point, Workspace::Direction direction) ^~~~~~~~~ In file included from /home/pedroarthur/repositories/kde/kwin/useractions.cpp:38:0: /home/pedroarthur/repositories/kde/kwin/workspace.h:499:10: note: declared private here enum Direction { ^~~~~~~~~ ^~~~~~~~~~~~~ Any suggestion? About the function, IMHO, it has a trivial implementation, but a obscure (non-trivial) purpose; it deserves a name. Also, I think that the callee is already big enough, so I wouldn't unroll it if I had my way. pedroarthurp: Got it for the static function, but I run into the following errors while trying to follow your… | |||||
Unrolling or using a lambda function would fix it. I don't like adding new methods to workspace as it tends to be a dumping ground. And that would be a method only called from one place. So even if the method is already looking in this case a lambda or unrolling might be the better option graesslin: Unrolling or using a lambda function would fix it. I don't like adding new methods to workspace… | |||||
1577 | if (!switchWindow(c, direction, curPos, desktopNumber)) { | ||||
1578 | auto opposite = [&] { | ||||
1579 | switch(direction) { | ||||
this method does not need to be a member of Workspace. You can just turn it into a static local method. graesslin: this method does not need to be a member of Workspace. You can just turn it into a static local… | |||||
or a closure or dropped altogether - afaics it's only called once and "opposite position" (opposite to what...) is a rather meaningless name as well. luebking: or a closure or dropped altogether - afaics it's only called once and "opposite position"… | |||||
1580 | case DirectionNorth: | ||||
1581 | return QPoint(curPos.x(), screens()->geometry().height()); | ||||
1582 | case DirectionSouth: | ||||
1583 | return QPoint(curPos.x(), 0); | ||||
1584 | case DirectionEast: | ||||
1585 | return QPoint(0, curPos.y()); | ||||
1586 | case DirectionWest: | ||||
the reference points you are calculating might be outside the visible area. graesslin: the reference points you are calculating might be outside the visible area. | |||||
There is no problem in this case. switchWindow will use it just as a reference to find the closest window to that point. pedroarthurp: There is no problem in this case. switchWindow will use it just as a reference to find the… | |||||
1587 | return QPoint(screens()->geometry().width(), curPos.y()); | ||||
1588 | default: | ||||
1589 | Q_UNREACHABLE(); | ||||
graesslin: please add a default case with Q_UNREACHABLE() and remove the return | |||||
1590 | } | ||||
1591 | }; | ||||
1592 | | ||||
1593 | switchWindow(c, direction, opposite(), desktopNumber); | ||||
1594 | } | ||||
1595 | } | ||||
1596 | | ||||
1597 | bool Workspace::switchWindow(AbstractClient *c, Direction direction, QPoint curPos, int d) | ||||
1598 | { | ||||
1599 | Client *switchTo = nullptr; | ||||
1600 | int bestScore = 0; | ||||
1601 | | ||||
1578 | ToplevelList clist = stackingOrder(); | 1602 | ToplevelList clist = stackingOrder(); | ||
1579 | for (ToplevelList::Iterator i = clist.begin(); i != clist.end(); ++i) { | 1603 | for (ToplevelList::Iterator i = clist.begin(); i != clist.end(); ++i) { | ||
1580 | Client *client = qobject_cast<Client*>(*i); | 1604 | Client *client = qobject_cast<Client*>(*i); | ||
1581 | if (!client) { | 1605 | if (!client) { | ||
1582 | continue; | 1606 | continue; | ||
1583 | } | 1607 | } | ||
1584 | if (client->wantsTabFocus() && *i != c && | 1608 | if (client->wantsTabFocus() && *i != c && | ||
1585 | client->isOnDesktop(d) && !client->isMinimized() && (*i)->isOnCurrentActivity()) { | 1609 | client->isOnDesktop(d) && !client->isMinimized() && (*i)->isOnCurrentActivity()) { | ||
Show All 33 Lines | 1641 | if (score < bestScore || !switchTo) { | |||
1619 | bestScore = score; | 1643 | bestScore = score; | ||
1620 | } | 1644 | } | ||
1621 | } | 1645 | } | ||
1622 | } | 1646 | } | ||
1623 | } | 1647 | } | ||
1624 | if (switchTo) { | 1648 | if (switchTo) { | ||
1625 | if (switchTo->tabGroup()) | 1649 | if (switchTo->tabGroup()) | ||
1626 | switchTo = switchTo->tabGroup()->current(); | 1650 | switchTo = switchTo->tabGroup()->current(); | ||
1627 | activateClient(switchTo); | 1651 | activateClient(switchTo); | ||
you could turn the payload of the function into a closure (Martin locves lambdas ;-) and call that with the original (and if required) flipflopped point. Martin's take, though. luebking: you could turn the payload of the function into a closure (Martin locves lambdas ;-) and call… | |||||
1628 | } | 1652 | } | ||
1653 | | ||||
1654 | return switchTo; | ||||
1629 | } | 1655 | } | ||
1630 | 1656 | | |||
1631 | /*! | 1657 | /*! | ||
1632 | Switches to upper window | 1658 | Switches to upper window | ||
1633 | */ | 1659 | */ | ||
1634 | void Workspace::slotSwitchWindowUp() | 1660 | void Workspace::slotSwitchWindowUp() | ||
1635 | { | 1661 | { | ||
1636 | switchWindow(DirectionNorth); | 1662 | switchWindow(DirectionNorth); | ||
▲ Show 20 Lines • Show All 224 Lines • Show Last 20 Lines |
given that it's only called from one method I think this should be turned into a lambda or static local method.