Review Request 115595: Enhancing zoom feature in Gwenview

[Aurélien Gâteau]

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


Hi, thanks for the patch!

The overall idea looks good, but I have some suggestions regarding the implementation. Some are coding style issues, others are slight improvements.


app/viewmainpage.cpp
<https://git.reviewboard.kde.org/r/115595/#comment34914>

    Coding style: Gwenview code uses 4 spaces for indentation, not tabs. Being consistent with indentation makes diff more readable.



app/viewmainpage.cpp
<https://git.reviewboard.kde.org/r/115595/#comment34911>

    DocumentView::Setup has a `valid` field, which could be used here to avoid looking up the url in `urlToSetup` twice. The code could look like this:
    
    DocumentView::Setup savedSetup = d->mDocumentViewContainer->savedSetup(url);
    view->openUrl(url, savedSetup.valid ? savedSetup : setup);



lib/documentview/documentviewcontainer.h
<https://git.reviewboard.kde.org/r/115595/#comment34904>

    coding style: space before the '<' character



lib/documentview/documentviewcontainer.h
<https://git.reviewboard.kde.org/r/115595/#comment34905>

    ditto



lib/documentview/documentviewcontainer.h
<https://git.reviewboard.kde.org/r/115595/#comment34912>

    No longer necessary with the suggestion I made above



lib/documentview/documentviewcontainer.h
<https://git.reviewboard.kde.org/r/115595/#comment34913>

    This method should be marked const because it does not modify DocumentViewContainer.



lib/documentview/documentviewcontainer.cpp
<https://git.reviewboard.kde.org/r/115595/#comment34906>

    I like to name maps or hashs the other way around. In this case it would be SetupForUrl.
    
    The reason behind this is it makes the code more readable, because the concepts are on the same side. I find this:
    
    Setup setup = setupForUrl(url);
    
    More readable than this:
    
    Setup setup = urlToSetup(url);



lib/documentview/documentviewcontainer.cpp
<https://git.reviewboard.kde.org/r/115595/#comment34907>

    Coding style: Member variables should be prefixed with 'm' (for "member")



lib/documentview/documentviewcontainer.cpp
<https://git.reviewboard.kde.org/r/115595/#comment34909>

    Coding style: no parenthesis around return values.



lib/documentview/documentviewcontainer.cpp
<https://git.reviewboard.kde.org/r/115595/#comment34908>

    I think this can be simplified into:
    
    d->urlToSetup[view->url()] = view->setup();


- Aurélien Gâteau


On Feb. 8, 2014, 10:22 p.m., Abhinav Gangwar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115595/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2014, 10:22 p.m.)
> 
> 
> Review request for Gwenview and Aurélien Gâteau.
> 
> 
> Repository: gwenview
> 
> 
> Description
> -------
> 
> The patch I designed will let Gwenview save the zoom configuration of current image, whenever the user switches to other image. Now if the other image has not been viewed before ( means no saved zoom configuration exists for this image), open it with the current zoom levels set by ZoomLock value (If ZoomLock is not enabled) or with its default size(if ZoomLock is not enabled). Otherwise display the image with its saved zoom configurations. After the Gwenview session ends, it will flush all these zoom configurations.
> 
>        The currently implemented ZoomLock feature allows to view all the images with same zoom level that has been set by user. Now, if a user switches to other image(say im2) from current image(say im1) and zoom it (to compare or some other purpose) with a different zoom level, he is gonna lost the previous image's(im1) zoom levels using which he wanted to compare the other image(im2). The user may need different zoom levels for different images to compare them. So, using the zoomlock feature, user still needs to change zoom levels again and again for images( becomes tedious for large number of images).
> 
> 
> Diffs
> -----
> 
>   app/viewmainpage.cpp b40b622 
>   lib/documentview/documentviewcontainer.h a4ef4a7 
>   lib/documentview/documentviewcontainer.cpp 475e7a8 
> 
> Diff: https://git.reviewboard.kde.org/r/115595/diff/
> 
> 
> Testing
> -------
> 
> I tested the patch on my system and it produced expected results.
> 
> 
> Thanks,
> 
> Abhinav Gangwar
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gwenview-devel/attachments/20140210/e816b7b1/attachment.html>