It seams that the user would need to be calling this function at the same time on more then one instance. How likely is that to happen? How do you feel about the code below? It removes the need for that variable all together. and removes the default case that will never be called.

void AtCore::move(AtCore::AXES axis, int arg)
{
    switch (axis) {
    case AtCore::X:
        move(QLatin1Char('X'), arg);
        break;
    case AtCore::Y:
        move(QLatin1Char('Y'), arg);
        break;
    case AtCore::Z:
        move(QLatin1Char('Z'), arg);
        break;
    case AtCore::E:
        move(QLatin1Char('E'), arg);
        break;
    };
}

In D14891#310391, @rizzitello wrote:

It seams that the user would need to be calling this function at the same time on more then one instance. How likely is that to happen? How do you feel about the code below? It removes the need for that variable all together. and removes the default case that will never be called.

Just imagine someone (like that eccentric rich cousin that everyone has :-)) using atcore to implement a client that is able to print to several printers at the same time. In order to AtCore to be used as a library, it needs to make no (or at least as few as possible) assumptions about how it is used (or at least document that it should not be used to control multiple printers).

void AtCore::move(AtCore::AXES axis, int arg)
{
    switch (axis) {
    case AtCore::X:
        move(QLatin1Char('X'), arg);
        break;
    case AtCore::Y:
        move(QLatin1Char('Y'), arg);
        break;
    case AtCore::Z:
        move(QLatin1Char('Z'), arg);
        break;
    case AtCore::E:
        move(QLatin1Char('E'), arg);
        break;
    };
}

It would work, but my experience says "never will be executed" never holds true for long, no matter how obvious it sounds. I agree that this code would work, but I personally don't like the style, as has some code repetition (the call to move(2). What do you think about this one?:

const auto a = [=]() -> QLatin1Char {
    switch (axis) {
    case AtCore::X:
        return QLatin1Char('X');
    case AtCore::Y:
        return QLatin1Char('Y');
    case AtCore::Z:
        return QLatin1Char('Z');
    case AtCore::E:
        return QLatin1Char('E');
    default:
        // maybe log it too
        Q_ASSERT("This should really never happen");
        return QLatin1Char('?');
    };
}();


if (a != QLatin1Char('?')) {
    move(a, arg);
}

But I agree this is a style that not everyone is used to and kind of diverges from the style usually used on AtCore.

One other idea is getting rid of the switch statement at all:

const static auto map = std::map<AtCore::AXES, QLatin1Char>{
    {AtCore::X, QLatin1Char('X')},
    {AtCore::Y, QLatin1Char('Y')},
    {AtCore::Z, QLatin1Char('Z')},
    {AtCore::E, QLatin1Char('E')},
};

const auto it = map.find(axis);

Q_ASSERT(it != end(map));

if (it != end(map)) {
    move(it->first, arg);
}

I have not tested any of the snippets above, they maybe don't even compile, but demonstrate the idea. Such ideas can even be applied in other parts of the codebase.

Otherwise I think just the original patch, removing the static keyword would be a good fix for now, as it changes as less code as possible, being less likely to introduce bugs than a bigger change.

Just imagine someone (like that eccentric rich cousin that everyone has :-)) using atcore to implement a client that is able to print to several printers at the same time. In order to AtCore to be used as a library, it needs to make no (or at least as few as possible) assumptions about how it is used (or at least document that it should not be used to control multiple printers).

Atelier already does allow you to print in this way and that would be fine since the files are sent directly this function is only used for a move command being generated by the client gui.

Code blocks

The second will be fine if it works. What if If we use the second part make a new function axisToChar()

QLatin1Char AtCore::axisToChar(AtCore::Axis axis)
{
    switch (axis) {
    case AtCore::X:
        return QLatin1Char('X');
    case AtCore::Y:
        return QLatin1Char('Y');
    case AtCore::Z:
        return QLatin1Char('Z');
    case AtCore::E:
        return QLatin1Char('E');
    default:
        // maybe log it too
        Q_ASSERT("This should really never happen");
        return QLatin1Char('?');
    };
}

Then our move can just be

void AtCore::move(AtCore::AXES axis, int arg)
{
   move(axisToChar(axis), arg);
}

void AtCore::move(QLatin1Char axis, int arg)
{
    if(axis == QLatin1Char('?')) {
        return;
    }
    pushCommand(GCode::toCommand(GCode::G1, QStringLiteral("%1 %2").arg(axis).arg(QString::number(arg))));
}

This way if we have a future need for this conversion we can use this function ,make it public so clients can use it too if need be then we only need to update this in one spot ever.

For everything that's being said here:
register as QEnum
use the toStirng method?

In D14891#310775, @tcanabrava wrote:

For everything that's being said here:
register as QEnum
use the toStirng method?

its already registered as a QEmum (all of our enums are)
I was unaware that was even a thing .. @leandrosantiago try that ...

Way better :)

Em sex, 17 de ago de 2018 às 21:05, Leandro Santiago <
noreply@phabricator.kde.org> escreveu:

leandrosantiago added inline comments. View Revision
https://phabricator.kde.org/D14891
*INLINE COMMENTS*
View Inline https://phabricator.kde.org/D14891#inline-79331
atcore.cpp:604
{
static QLatin1Char a('?');
switch (axis) {
case AtCore::X:
a = QLatin1Char('X');
break;
case AtCore::Y:
a = QLatin1Char('Y');
break;
case AtCore::Z:
a = QLatin1Char('Z');
break;
case AtCore::E:
a = QLatin1Char('E');
break;
default:
break;
};
move(a, arg);
const auto axisAsString = QMetaEnum::fromType<decltype(axis)>().valueToKey
(axis);
move(QLatin1Char(axisAsString[0]), arg);

I haven't really tested it. Please let me know if this is invalid use of
the QMetaEnum.

View Inline https://phabricator.kde.org/D14891#inline-79332
atcore.cpp:605
static QLatin1Char a('?');
switch (axis) {
case AtCore::X:
a = QLatin1Char('X');
break;
case AtCore::Y:
a = QLatin1Char('Y');
break;
case AtCore::Z:
a = QLatin1Char('Z');
break;
case AtCore::E:
a = QLatin1Char('E');
break;
default:
break;
};
move(a, arg);
const auto axisAsString = QMetaEnum::fromType<decltype(axis)>().valueToKey
(axis);
move(QLatin1Char(axisAsString[0]), arg);
}

axisAsString here will always be a non-null string, if axis ls always of
type AXES. In case it isn't, that's the caller's fault, and must be handled
by it.

*REPOSITORY*
R232 AtCore

*REVISION DETAIL*
https://phabricator.kde.org/D14891

*To: *leandrosantiago, rizzitello, patrickelectric, laysrodrigues,
tcanabrava
*Cc: *patrickelectric, leandrosantiago, rizzitello

Yes, if someone is forcing a int is because he knows what he is doing (or
not).
In either case, if you are using a int as argument the compiler will not
accept.