View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0015205 | Scribus | General | public | 2018-03-20 13:37 | 2018-03-21 09:54 |
Reporter | ale | Assigned To | |||
Priority | normal | Severity | minor | Reproducibility | always |
Status | new | Resolution | open | ||
Product Version | 1.5.4.svn | ||||
Summary | 0015205: horizontally swapping does not keep the vertical position + add vertical swapping | ||||
Description | i'm not sure of all possible uses of the swapping tools. but in my use case (two objects of different size that had to exchange place, it's a bit odd (and makes the tool useless for me in this case) to modify the y coordinates of the moved item. (see attached gif) well, in my case, i would have needed a vertical swap... but i tried anyway before filling this ticket for the vertical swapping : - ) so, can we also have a vertical swapping? | ||||
Tags | No tags attached. | ||||
Patch | No | ||||
|
|
|
i try to add the vertical swapping... if i see that fixing the swapping is trivial i will do that too... |
|
ok, now i see a different case, where it works correctly... |
|
ok, so, despite the icon, it already swaps also vertically... in some cases it does what the user expects, in others not... i wonder what option there should be to also make me happy in the first case... |
|
i've tried it out... and i think that i've learnt a few things: - the current swapLeft and swapRight functions do not work in every case, contain dead code and are doing it in a way too complex way. - the current swapLeft / swapRight are in reality a cycleLeft and cycleRight function (123 -> 312 -> 231 -> 123) - there is probably an issue with AObjects: the Objects list it contains will always contain one single PageItem... but i'm not 100% sure of it... - ... if this is the case, i wonder if AObjects is really needed and if it the code would not be clearer if the align&distribute functions would directly work on a QList<PageItem*>. (there also might be bugs related to it: as an example, swapLeft uses AObjects[i].x but modifies AObjects[i].objects[0]->x, without refreshing the coordinates in AObject[i].) - finally: why are the align&distribute function in ScribusDoc and not in an own class? refactoring them out would be a step in making ScribusDoc a little bit less of a huge monster... i suggest that - we only keep the swapRight function and rename it with "cycle" (in most (or every?) cases you will be only swapping very few elements... and you can press up to 10 times on the button... it's not a function somebody uses that often... and it's imo important that the feature is easy to grasp) - we add a a swap that does: 12345 -> 54321 -> 12345 - eventually refactor (out) AObjects (and all the align&distribute functions) finally, i have a swapLeft function that is much shorter and more explicit on what it does (20% of the code... and does almost the same thing) i prepare a "real" swap and then propose a patch for both of them. |
|
ok, i have a proof of concept for the cycle and swap i will now try to have a look at the number of Objects in AObjects... cycle-swap.cpp (2,276 bytes)
#include <QDebug> #include <QList> class AObject; class AObject { public: char t{' '}; int x{0}; int y{0}; }; class ScribusDoc { public: void swap() { QList<AObject*> items{}; for (auto &a: AObjects) { items.append(&a); } qSort(items.begin(), items.end(), compareAObject); auto itBegin = items.begin(); auto itEnd = items.end(); --itEnd; while (itBegin < itEnd) { auto swapX = (*itBegin)->x; auto swapY = (*itBegin)->y; (*itBegin)->x = (*itEnd)->x; (*itBegin)->y = (*itEnd)->y; (*itEnd)->x = swapX; (*itEnd)->y = swapY; ++itBegin; --itEnd; } } void cycleRight() { QList<AObject*> items{}; for (auto &a: AObjects) { items.append(&a); } qSort(items.begin(), items.end(), compareAObject); auto it = items.begin(); auto swapX = (*it)->x; auto swapY = (*it)->y; auto prevIt = it; ++it; while (it != items.end()) { (*prevIt)->x = (*it)->x; (*prevIt)->y = (*it)->y; prevIt = it; ++it; } it = --items.end(); (*it)->x = swapX; (*it)->y = swapY; } void render() { qDebug() << "-----"; for (auto a: AObjects) { qDebug() << a.t << a.x << a.y; } } private: QList<AObject> AObjects{{'c',3,4},{'a',2,1},{'b',2,2}}; static bool compareAObject(const AObject* a, const AObject* b) { return a->x < b->x || (a->x == b->x && a->y < b->y); } }; int main() { ScribusDoc doc; doc.render(); qDebug() << "a b c > c a b > b c a > a b c"; doc.cycleRight(); doc.render(); doc.cycleRight(); doc.render(); doc.cycleRight(); doc.render(); qDebug() << "a b c > c a b > a b c"; doc.swap(); doc.render(); doc.swap(); doc.render(); } |
|
i've created a gist (with the CMakeFile), for those who want to try out: https://gist.github.com/aoloe/d98bc9ec0e88e65600908768db75ea18 |
Date Modified | Username | Field | Change |
---|---|---|---|
2018-03-20 13:37 | ale | New Issue | |
2018-03-20 13:37 | ale | File Added: swap.gif | |
2018-03-20 13:59 | ale | Note Added: 0045061 | |
2018-03-20 17:03 | jghali | Category | - => General |
2018-03-20 18:41 | ale | File Added: 1-2-3-swap.gif | |
2018-03-20 18:41 | ale | Note Added: 0045064 | |
2018-03-20 18:48 | ale | Note Added: 0045065 | |
2018-03-21 09:01 | ale | Note Added: 0045066 | |
2018-03-21 09:30 | ale | File Added: cycle-swap.cpp | |
2018-03-21 09:30 | ale | Note Added: 0045067 | |
2018-03-21 09:54 | ale | Note Added: 0045068 |