Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dimradial draw function #42

Merged
merged 1 commit into from
Apr 10, 2016
Merged

Update dimradial draw function #42

merged 1 commit into from
Apr 10, 2016

Conversation

bhattigurjot
Copy link
Contributor

No description provided.

painter.line_to(tLinePos.x(), mousePos.y());
painter.stroke();
endCaps.render(painter, EndCaps::CLOSEDARROW, mousePos.x(), mousePos.y(), definitionPoint2().x(), definitionPoint2().y(), capSize) ;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to cleanup a little bit here, so the code reads easier:

    using namespace lc::geo;

    Coordinate offsetX{te.width + capSize, 0};
    Coordinate offsetY{0, te.width/4 + capSize/2};

    if (textisInside && textFitsInside) {
    auto const offset = (definitionPoint2().x() > mousePos.x()) ? - offsetX : offsetX;
    auto const tLinePos = mousePos + offset;
    auto const tMText = mousePos + offset/2;

    if  (definitionPoint2().y() > mousePos.y()) {
        tMText -= offsetY;
    }

    //the painter code might read better with some adaptive interface
    auto move_to = [&painter] (Coordinate const& coord) {painter.move_to(coord.x, coord.y);}
    auto line_to = [&painter] (Coordinate const& coord) {painter.line_to(coord.x, coord.y);}

        move_to(definitionPoint2());
        line_to(mousePos);

the logic can be further simplified as far as the intention is still clear enough to be read from code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dli,

perhaps we can create some header for that adaptive interface with some generic (wildly guessing).
I wanted to keep our entities (Coordinate etc... etc) out of the painter functions so we don't have potential circular references, or at a minimum that the painter doesn't depend on the geo classes. This is why I didn't add the Coordinate classes as method paramaters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

mixing geometry/logic and dependency of painter interface seems to be bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to make such changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bhattigurjot
please make your own judgement call, and avoid getting into the "deadlock" situation, as I explained early on: while one party is waiting for comments, and the other party is waiting to see actions.

Thanks!

@rvt rvt merged commit 4d4a703 into LibreCAD:master Apr 10, 2016
@XVilka XVilka mentioned this pull request Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants