View Issue Details

IDProjectCategoryView StatusLast Update
0015205ScribusGeneralpublic2018-03-21 09:54
Reporterale Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Product Version1.5.4.svn 
Summary0015205: horizontally swapping does not keep the vertical position + add vertical swapping
Descriptioni'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?
TagsNo tags attached.
PatchNo

Activities

ale

2018-03-20 13:37

manager  

swap.gif (63,037 bytes)   
swap.gif (63,037 bytes)   

ale

2018-03-20 13:59

manager   ~0045061

i try to add the vertical swapping... if i see that fixing the swapping is trivial i will do that too...

ale

2018-03-20 18:41

manager   ~0045064

ok, now i see a different case, where it works correctly...
1-2-3-swap.gif (34,577 bytes)   
1-2-3-swap.gif (34,577 bytes)   

ale

2018-03-20 18:48

manager   ~0045065

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...

ale

2018-03-21 09:01

manager   ~0045066

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.

ale

2018-03-21 09:30

manager   ~0045067

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();
}
cycle-swap.cpp (2,276 bytes)   

ale

2018-03-21 09:54

manager   ~0045068

i've created a gist (with the CMakeFile), for those who want to try out:

https://gist.github.com/aoloe/d98bc9ec0e88e65600908768db75ea18

Issue History

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