[Kstars-devel] Review Request 116965: Added exponential zoom in/out feature in Solar System Viewer

[Rafal Kulaga]

> On April 11, 2014, 10:50 a.m., Rafal Kulaga wrote:
> > kstars/tools/pvplotwidget.cpp, line 252
> > <https://git.reviewboard.kde.org/r/116965/diff/3/?file=263397#file263397line252>
> >
> >     It's better not to use this when it is not required.
> 
> Vijay Dhameliya wrote:
>     It's needed here to update factor.

Well, I've meant the "this" pointer ;-) We don't use it in KStars when it is not required.


> On April 11, 2014, 10:50 a.m., Rafal Kulaga wrote:
> > kstars/tools/pvplotwidget.cpp, line 275
> > <https://git.reviewboard.kde.org/r/116965/diff/3/?file=263397#file263397line275>
> >
> >     Why should this be random?
> 
> Vijay Dhameliya wrote:
>     To produce coarse zoom factor, one can always argument against any particular value I use. 
>     If you still think it should not be random then what value should I use here ?

Yeah, but it will make zoom work differently each time. That's not convenient for the user. I think that you can choose anything from 20-30 instead of this random factor. It will be a much better idea.


> On April 11, 2014, 10:50 a.m., Rafal Kulaga wrote:
> > kstars/tools/pvplotwidget.h, line 53
> > <https://git.reviewboard.kde.org/r/116965/diff/3/?file=263396#file263396line53>
> >
> >     I think that this function should return magFactor value, instead of modifying the factor field - it's just more elegant.
> 
> Vijay Dhameliya wrote:
>     Yeah it would be, but if I make it return type function then where would I call it ? and how would I check whether the Ctrl is pressed or not ?

I am not sure if I understood you correctly, but if you insist on making it this way, at least change the name of the function to something like updateMagFactor() - most of the time, I expect functions named magFactor(), name(), surname() etc. to return appropriate data, not silently update them.


- Rafal


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116965/#review55447
-----------------------------------------------------------


On April 8, 2014, 4:20 a.m., Vijay Dhameliya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116965/
> -----------------------------------------------------------
> 
> (Updated April 8, 2014, 4:20 a.m.)
> 
> 
> Review request for KStars and Rafal Kulaga.
> 
> 
> Repository: kstars
> 
> 
> Description
> -------
> 
> Currently zoom in/out factor is set constant which is very small so zoom in/out works very slowly, i.e. user have to keep rolling mouse wheel for long to get zoom enough to observe single planet. 
> 
> This features allows user to do quick zoom in/out by pressing ctrl + wheel/+-keys. 
> 
> 
> Diffs
> -----
> 
>   kstars/tools/pvplotwidget.cpp b8a8b30 
>   kstars/tools/pvplotwidget.h ef90ace 
> 
> Diff: https://git.reviewboard.kde.org/r/116965/diff/
> 
> 
> Testing
> -------
> 
> Testing has done for following use cases:
> 1) Zoom in/out by pressing +/- continuously till it can not zoom in/out furthermore 
> 2) Zoom in/out by rolling rolling wheel up/down continuously till it can not zoom in/out furthermore 
> 3) Zoom in/out by pressing Ctrl + +/- continuously till it can not zoom in/out furthermore 
> 4) Zoom in/out by rolling Ctrl + rolling wheel up/down continuously till it can not zoom in/out furthermore 
> 5) Zoom in/out by pressing +/- step by step till it can not zoom in/out furthermore 
> 6) Zoom in/out by rolling rolling wheel up/down step by step till it can not zoom in/out furthermore 
> 7) Zoom in/out by pressing Ctrl + +/- step by step till it can not zoom in/out furthermore 
> 8) Zoom in/out by rolling Ctrl + rolling wheel up/down step by step till it can not zoom in/out furthermore
> 
> 
> Thanks,
> 
> Vijay Dhameliya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kstars-devel/attachments/20140412/4d7d00c7/attachment.html>