[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    Re: KDE/kdegames/libkdegames
From:       Dmitry Suzdalev <dimsuzkde () gmail ! com>
Date:       2009-11-26 10:34:20
Message-ID: 200911261334.21158.dimsuz () gmail ! com
[Download RAW message or body]

Ah, one more thing!

Stefan, can you please add an explicit comment to that "if" you added?
Something stating why it was added and why it should stay there :)

something along the lines of:
// This check is needed to prevent endless looping between
// setText => update() => paintEvent() => setText

Through my experience in developing large projects which involve many 
developers working on a same code (kde being one of them) I can say that such 
comments tend to be VERY helpful in future. Not only for others, but for you 
too :) Occasionally you'll forget this commit of yours and in case you don't 
look up every line you change with svn blame, you can delete/change this and 
it'll break again. Another point is that this comment will notify you/others 
of  a potential bug to look out.

I really find that this minor thing (a comment about fixing not-trivial bug) 
sometimes saves a lot of frustration, so I always comment such changes of mine 
:)

So I can say i have the following rule:

If you fixed a bug and the change in code you introduced doesn't speak for 
itself about what it fixes, this should be stated explicitly as a comment.

Cheers,
Dmitry.

On Thursday 26 November 2009 12:43:11 Stefan Majewsky wrote:
> SVN commit 1054518 by majewsky:
> 
> Fix coding style.
> CCMAIL: iandw.au@gmail.com
> 
> 
>  M  +32 -2     kgamepopupitem.cpp
> 
> 
> --- trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp #1054517:1054518
> @@ -68,7 +68,9 @@
>      //this check may result in an infinite loop of paintEvent ->
>  setDefaultTextColor -> update -> paintEvent... const QColor textColor =
>  m_brush.brush(widget).color();
>      if (textColor != defaultTextColor())
> +    {
>          setDefaultTextColor(textColor);
> +    }
>      //render contents
>      p->save();
>      p->setOpacity(m_opacity);
> @@ -215,9 +217,13 @@
>      p->setPen(pen);
> 
>      if( d->m_animOpacity != -1) // playing Center animation
> +    {
>          p->setOpacity(d->m_animOpacity);
> +    }
>      else
> +    {
>          p->setOpacity(d->m_opacity);
> +    }
>      p->setBrush(d->m_brush.brush(widget));
>      p->drawPath(d->m_path);
>      p->drawPixmap( MARGIN, static_cast<int>(d->m_boundRect.height()/2) -
>  d->m_iconPix.height()/2, @@ -230,9 +236,13 @@
>      if(d->m_timeLine.state() == QTimeLine::Running ||
>  d->m_timer.isActive()) {
>          if (mode == ReplacePrevious)
> +        {
>              forceHide(InstantHide);
> +        }
>          else
> +        {
>              return;// we're already showing a message
> +        }
>      }
> 
>      // NOTE: we blindly take first visible view we found. I.e. we don't
>  support @@ -245,7 +255,9 @@
>          }
>      }
>      if (!sceneView)
> +    {
>          sceneView = scene()->views().at(0);
> +    }
> 
>      QPolygonF poly = sceneView->mapToScene(
>  sceneView->viewport()->contentsRect() ); d->m_visibleSceneRect =
>  poly.boundingRect();
> @@ -261,7 +273,9 @@
>      qreal w =
>  d->m_textChildItem->boundingRect().width()+MARGIN*2+d->m_iconPix.width()+S
> OME_SPACE; qreal h = d->m_textChildItem->boundingRect().height()+MARGIN*2;
>  if( d->m_iconPix.height() > h )
> +    {
>          h = d->m_iconPix.height() + MARGIN*2;
> +	}
>      d->m_boundRect = QRectF(0, 0, w, h);
> 
>      // adjust to take into account the width of the pen
> @@ -334,9 +348,13 @@
>  void KGamePopupItem::animationFrame(int frame)
>  {
>      if( d->m_position == TopLeft || d->m_position == BottomLeft )
> +    {
>          setPos( d->m_visibleSceneRect.left()+SHOW_OFFSET, frame );
> +    }
>      else if( d->m_position == TopRight || d->m_position == BottomRight )
> +    {
>          setPos(
>  d->m_visibleSceneRect.right()-d->m_boundRect.width()-SHOW_OFFSET, frame );
>  +    }
>      else if( d->m_position == Center )
>      {
>          d->m_animOpacity = frame*d->m_opacity/d->m_timeLine.duration();
> @@ -348,8 +366,9 @@
>  void KGamePopupItem::playHideAnimation()
>  {
>      if( d->m_hoveredByMouse )
> +    {
>          return;
> -
> +    }
>      // let's hide
>      d->m_timeLine.setDirection( QTimeLine::Backward );
>      d->m_timeLine.start();
> @@ -410,7 +429,9 @@
>      d->m_hoveredByMouse = false;
> 
>      if( d->m_timeout != 0 && !d->m_timer.isActive() &&
>  d->m_timeLine.state() != QTimeLine::Running ) +    {
>          playHideAnimation(); // let's hide
> +	}
>  }
> 
>  void KGamePopupItem::setMessageIcon( const QPixmap& pix )
> @@ -428,7 +449,9 @@
>  void KGamePopupItem::forceHide(HideType howToHide)
>  {
>      if(!isVisible())
> +    {
>          return;
> +    }
> 
>      if(howToHide == InstantHide)
>      {
> @@ -466,12 +489,15 @@
>  void KGamePopupItem::onLinkHovered(const QString& link)
>  {
>      if(link.isEmpty())
> +    {
>          d->m_textChildItem->setCursor( Qt::ArrowCursor );
> +    }
>      else
> +    {
>          d->m_textChildItem->setCursor( Qt::PointingHandCursor );
> +    }
> 
>      d->m_linkHovered = !link.isEmpty();
> -
>      emit linkHovered(link);
>  }
> 
> @@ -498,14 +524,18 @@
>      // special signal to indicate mouse click which we catch in a
>  onTextItemClicked() // slot
>      if (d->m_hideOnClick)
> +    {
>          forceHide();
> +    }
>  }
> 
>  void KGamePopupItem::onTextItemClicked()
>  {
>      // if link is hovered we don't hide as click should go to the link
>      if (d->m_hideOnClick && !d->m_linkHovered)
> +    {
>          forceHide();
> +    }
>  }
> 
>  #include "moc_kgamepopupitem.cpp" // For automocing KGamePopupItem
> 
[prev in list] [next in list] [prev in thread] [next in thread]