D27094: Multiple cell selection + new control element for entries + first version of selection actions

[Alexander Semke]

asemke added inline comments.

INLINE COMMENTS

> CMakeLists.txt:82
>     extended_document.cpp
> +   worksheetcontrolelement.cpp
>   )

to be more consistent with other classes, let's name the new files worksheetcontrolitem.h/cpp and the class WorksheetControlItem.

> actionbar.cpp:34
>      QPointF p = worksheet()->worksheetView()->viewRect().topRight();
> -    qreal w = qMin(parent->size().width(),
> +    qreal w = qMin(parent->size().width()-WorksheetEntry::RightMargin,
>                     parent->mapFromScene(p).x());

spaces around the operators are not consistently used in Cantor's code nor in this patch. Let's consistently use spaces around every operator.

> worksheet.cpp:1541
> +    if (m_selectedEntries.isEmpty())
> +    {
> +        WorksheetEntry* entry = entryAt(pos);

unnecessary blank.

> worksheet.cpp:1687
> +        {
> +            for (WorksheetEntry* entry: m_selectedEntries)
> +            {

space in front of :

> worksheet.cpp:2434
> +{
> +    for (WorksheetEntry* entry: m_selectedEntries)
> +        if (isValidEntry(entry))

space if front of :

> worksheet.cpp:2444
> +    // run entries in worksheet order: from top to down
> +    for(WorksheetEntry* entry = firstEntry(); entry; entry = entry->next())
> +        if(m_selectedEntries.indexOf(entry) != -1)

space in front of the opening bracket (

> worksheet.h:277
> +    //Actions for selection
> +    void SelectionRemove();
> +    void SelectionEvaluate();

function names start with a small letter.

> worksheetcontrolelement.cpp:25
> +#include <QApplication>
> +#include <QDebug>
> +

the debug output below doesn't see to be relevant anymore.

> worksheetcontrolelement.cpp:70
> +
> +    if (isSelected)
> +    {

similar to how we visualize the selection of the control item we should also visualize the item when it's being hovered. Let's introduce a new variable m_overed here and handle it appropriately in hoverEnterEvent() and in hoverLeaveEvent().

> worksheetcontrolelement.cpp:78
> +
> +        painter->fillRect(rect(), brush);
> +    }

painter->fillRect(rect(), QApplication::palette().color(QPalette::Highlight) would also do the job I think.

> worksheetcontrolelement.cpp:91
> +    qDebug() << "WorksheetControlElement::mouseMoveEvent";
> +    const QPointF buttonDownPos = event->buttonDownPos(Qt::LeftButton);
> +    if (event->buttons() == Qt::LeftButton && contains(buttonDownPos) && (event->pos() - buttonDownPos).manhattanLength() >= QApplication::startDragDistance())

this assignment and the evaluation of buttonDownPos() is only needed if we have Qt::LeftButton. I'd use

  if (event->buttons() != Qt::LeftButton)
      return;
  
  const QPointF downPos = event->buttonDownPos(Qt::LeftButton);
  if (contains(downPos) && (event->pos() - downPos).manhattanLength() >= QApplication::startDragDistance())
  {
  ...
  }

> worksheetcontrolelement.h:36
> +    void doubleClick();
> +    void drag(const QPointF&, const QPointF&);
> +

for QPointF it's better/faster to work with object copies and not with references.

> worksheetcontrolelement.h:40
> +    void mouseMoveEvent(QGraphicsSceneMouseEvent* event) override;
> +    void mouseDoubleClickEvent( QGraphicsSceneMouseEvent * event ) override;
> +    void paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget = nullptr) override;

inconsistent usage of spaces here.

> worksheetcontrolelement.h:44
> +  public:
> +    bool isSelected{false};
> +    bool isCollapsable{false};

these is* variables sound like function names. Let's consistently name private variables m_* like in WorksheetTextItem, etc.

REPOSITORY
  R55 Cantor

REVISION DETAIL
  https://phabricator.kde.org/D27094

To: sirgienko, asemke
Cc: kde-edu, asemke, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20200202/25d1309c/attachment-0001.html>