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

List:       kde-edu-devel
Subject:    Re: Review Request 114692: Better Plotter2D
From:       Percy Camilo Triveño Aucahuasi <percy.camilo.ta () gmail ! 
Date:       2013-12-31 6:04:53
Message-ID: 52C25E85.9080308 () gmail ! com
[Download RAW message or body]

Hi,

On 29/12/13 19:45, Aleix Pol Gonzalez wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114692/
> 
> 
> Ship it!
> 
> In general it looks good to me, once this issues have been sorted out, you can \
> commit. (But please, sort them out) 
> 
> analitzagui/plotsview2d.h
> <https://git.reviewboard.kde.org/r/114692/diff/1/?file=227422#file227422line127>
> (Diff revision 1)
> 
> public slots:
> 
> 	
> 
> 	116 	
> 
> void  zoomIn()  {  Plotter2D::zoomIn(true);  }
> 
> Seems like you want these methods virtual.
> 
> 
> analitzaplot/plotter2d.cpp
> <https://git.reviewboard.kde.org/r/114692/diff/1/?file=227426#file227426line882>
> (Diff revision 1)
> 
> void Plotter2D::drawFunctions(QPaintDevice *qpd)
> 
> 492 	
> 
> QPointF  ultim(toWidget(vect[0]));
> 
> 	825 	
> 
> QPointF  ultim(toWidget(vect.at(0)));
> 
> Why did you change that?
> 

at() can be faster than operator[](), because it never causes a deep 
copy to occur.


> 
> analitzaplot/plotter2d.cpp
> <https://git.reviewboard.kde.org/r/114692/diff/1/?file=227426#file227426line1154>
> (Diff revision 1)
> 
> void Plotter2D::setXAxisLabel(const QString &label)
> 
> 745 	
> 
> m_ticksFormat  =  tsfmt;
> 
> 	1091 	
> 
> scaleViewport(ZoomInFactor,  QPoint(m_size.width()*0.5,  m_size.height()*0.5),  \
> repaint); 
> This changes behavior. Why?
> 

The last one was buggy, if you see the description in the previous 
version it says: "FIXME:Bad solution" the one that I'm proposing is the 
standard behavior for zooming and I think is bug free.

> 
> analitzaplot/plottingenums.h
> <https://git.reviewboard.kde.org/r/114692/diff/1/?file=227428#file227428line37>
> (Diff revision 1)
> 
> enum CoordinateSystem {
> 
> 	
> 
> 35 	
> 
> enum  CoordinateSystem  {
> 
> 	37 	
> 
> enum  CoordinateSystem
> 
> Unrelated changes, please don't commit those brace movements
> 

No, that is a good change, we need to have uniform code style, please 
don't comment about non important aspects of the patch.

> 
> - Aleix Pol Gonzalez
> 

Percy

> 
> On December 28th, 2013, 8:09 a.m. UTC, Percy Camilo Triveño Aucahuasi wrote:
> 
> Review request for KDE Edu and Aleix Pol Gonzalez.
> By Percy Camilo Triveño Aucahuasi.
> 
> /Updated Dec. 28, 2013, 8:09 a.m./
> 
> *Repository: * analitza
> 
> 
> Description
> 
> - Fix many bugs (ticks, labels, zooming, etc,)
> - Add polar angles and polar axis.
> - Add option to draw the angles in radians, degrees and gradians.
> - Add new grid styles (some similar to KmPlot like crosses)
> - Better API to manage colors and to set render options (like drawticks, etc.)
> - Many minor fixes/improvements too.
> 
> 
> Testing
> 
> All test pass, all analitzaplot demos run ok
> 
> 
> Diffs
> 
> * analitzaplot/plottingenums.h (f0c4c1c)
> * analitzaplot/private/utils/mathutils.h (08630b8)
> * analitzaplot/private/utils/mathutils.cpp (d44ed6d)
> * analitzagui/plotsview2d.h (b08a716)
> * analitzagui/plotsview2d.cpp (f66f22d)
> * analitzaplot/plotitem.h (efe942c)
> * analitzaplot/plotter2d.h (ac52684)
> * analitzaplot/plotter2d.cpp (48085f1)
> * analitzaplot/plotter3d.h (85c3378)
> 
> View Diff <https://git.reviewboard.kde.org/r/114692/diff/>
> 
_______________________________________________
kde-edu mailing list
kde-edu@mail.kde.org
https://mail.kde.org/mailman/listinfo/kde-edu


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