Skip to content

Commit

Permalink
add TODOs to rename pos() and ipos()
Browse files Browse the repository at this point in the history
Element::pos() and Element::ipos() unclear and error prone semantic led to 382b7d7.
SpannerSegment::pos2() and SpannerSegment::ipos2() are not good as well.
I didn't apply renaming because active work is in progress.
  • Loading branch information
anatoly-os committed Sep 18, 2018
1 parent 9037e5e commit 588a4ba
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 0 deletions.
2 changes: 2 additions & 0 deletions libmscore/element.h
Expand Up @@ -198,7 +198,9 @@ class Element : public ScoreElement {
bool generated() const { return flag(ElementFlag::GENERATED); }
void setGenerated(bool val) { setFlag(ElementFlag::GENERATED, val); }

//TODO: rename to pos()
const QPointF& ipos() const { return _pos; }
//TODO: rename to posWithUserOffset()
virtual const QPointF pos() const { return _pos + _userOff; }
virtual qreal x() const { return _pos.x() + _userOff.x(); }
virtual qreal y() const { return _pos.y() + _userOff.y(); }
Expand Down
2 changes: 2 additions & 0 deletions libmscore/spanner.h
Expand Up @@ -82,7 +82,9 @@ class SpannerSegment : public Element {
qreal& rUserYoffset2() { return _userOff2.ry(); }

void setPos2(const QPointF& p) { _p2 = p; }
//TODO: rename to spanSegPosWithUserOffset()
QPointF pos2() const { return _p2 + _userOff2; }
//TODO: rename to spanSegPos()
const QPointF& ipos2() const { return _p2; }
qreal& rxpos2() { return _p2.rx(); }
qreal& rypos2() { return _p2.ry(); }
Expand Down

2 comments on commit 588a4ba

@wschweer
Copy link
Contributor

Choose a reason for hiding this comment

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

pos() is much more often used than ipos(). Therefore i suggest to change the semantic of _pos:

  • _pos should become (_pos + _userOff) to avoid the add operation for most cases
  • the new _pos is accessed as pos() as usual
  • ipos is defined as (pos() - userOff()) and can be named as you like (basePosition() ?)

@anatoly-os
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. These changes need a lot of renaming and accurate changing the code, but it will make code clearer.

Please sign in to comment.