D16721: Use Krita toolbar in Karbon

[Anthony Fieroni]

anthonyfieroni added a comment.


  Can you test my suggestions, it's looks good to me.

INLINE COMMENTS

> ognarb wrote in KoToolBoxDocker.cpp:39
> I found out that this QLabel is, it's the small draggable zone at the top of the docker. Actual it's still a bit buggy in krita and karbon F6397172: krita_toolbar_to.png <https://phabricator.kde.org/F6397172>. I should fix this before commiting. :D
> 
> Edit: It's not a bug, it's a feature :D If I try to center the label, it's look a lot less good looking then not zooming on it. I added a small comment for future reference. :)

Please remove label and return KoDockWidgetTitleBar it looks better.

> KoToolBoxLayout_p.h:221-228
> +        // Prefer showing two rows/columns by default
> +        QSize twoIcons = static_cast<Section*> (m_sections[0]->widget())->iconSize() * 2;
> +        const int length = doLayout(QRect(QPoint(), twoIcons), false);
> +        if (m_orientation == Qt::Vertical) {
> +            return QSize(twoIcons.width(), length);
> +        } else {
> +            return QSize(length, twoIcons.height());

Try this code

  const QSize minSize = minimumSize();
  if (!minSize.isValid()) {
      return minSize;
  }
  if (m_orientation == Qt::Vertical) {
      return QSize(minSize.width(), minSize.height() * 2 + spacing());
  } else {
      return QSize(minSize.height() * 2 + spacing(), minSize.width());
  }

> KoToolBoxLayout_p.h:270
> +        QLayout::setGeometry(rect);
> +        doLayout(rect, true);
> +    }

We don't need rect so

  doLayout(rect.size(), true);

> KoToolBoxLayout_p.h:281-282
> +        if (m_orientation == Qt::Vertical) {
> +            const int height = doLayout(QRect(0, 0, width, 0), false);
> +            return height;
> +        } else {

return doLayout(QSize(width, 0), false);

> KoToolBoxLayout_p.h:296-297
> +        if (m_orientation == Qt::Horizontal) {
> +            const int width = doLayout(QRect(0, 0, 0, height), false);
> +            return width;
> +        } else {

return doLayout(QSize(0, height), false);

> KoToolBoxLayout_p.h:310
> +private:
> +    int doLayout(const QRect &rect, bool notDryRun) const
>      {

Rename notDryRun to applyGeometry

> KoToolBoxLayout_p.h:333
>          bool firstSection = true;
>          foreach (QWidgetItem *wi, m_sections) {
>              Section *section = static_cast<Section*> (wi->widget());

I prefer to be 2 distinct loops

  if (!applyGeometry) {
      foreach (QWidgetItem *wi, m_sections) {
          Section *section = static_cast<Section*> (wi->widget());
          const int buttonCount = section->visibleButtonCount();
          if (buttonCount == 0) {
              continue;
          }
  
          // rows needed for the buttons (calculation gets the ceiling value of the plain div)
          const int neededRowCount = ((buttonCount - 1) / maxColumns) + 1;
  
          if (firstSection) {
              firstSection = false;
          } else {
              // start on a new row, set separator
              y += iconHeight + spacing();
          }
          // advance by all but the last used row
          y += (neededRowCount - 1) * iconHeight;
      }
      return y + iconHeight;
  }

and other one without notDryRun.

REPOSITORY
  R8 Calligra

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

To: ognarb, #krita, #calligra:_3.0
Cc: anthonyfieroni, Calligra-Devel-list, dcaliste, cochise, vandenoever
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20181109/6d977a8f/attachment.htm>