Review Request: Enable or disable "Enlarge to Fit" from the "Fit" button in the Gwenview statusbar

Discussion:

Todd

2010-03-31 02:56:42 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/
-----------------------------------------------------------

Review request for Gwenview.

Summary
-------

Currently in gwenview in order to disable or disable "Enlarge to fit" you must go to the gwenview configuration screen and enable or disable a checkbox. This patch adds an additional, much quicker method to accomplish the same goal. Pressing and holding on the "Fit" button in the statusbar now brings up a menu that allows you to switch between the two modes. The setting is remembered between sessions and is the same as the one in the configuration dialog, so changing it in one place also changes it in the other.

Diffs
-----

/trunk/KDE/kdegraphics/gwenview/lib/documentview/abstractdocumentviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/imageview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.cpp 1109333

Diff: http://reviewboard.kde.org/r/3452/diff

Testing
-------

Thanks,

Todd

Todd

2010-03-31 22:22:52 UTC

Permalink

Post by Todd

Thanks for posting it on reviewboard.
I just gave it a try, and I must confess I am not really fond of it: it feels weird to have to hold the "Fit" button to get the menu. I don't expect users to find out about this. Maybe we need a separate "Config" button to the left of the "Fit" button?

It would be more obvious if there were a little arrow (or actually a triangle) like there is on most buttons that you can hold down to bring up a menu. It is supposed to appear automatically, but didn't (probably due to the subclassing). If it could be added to the button manually I think that would be the best solution, but I don't know how to do that.

If we are going to add another button, I think it would be better to go with my original suggestion and have a separate button that would enlarge or shrink. Then people wouldn't need to worry about configuring and the configuration option could be removed entirely.

- Todd

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4822
-----------------------------------------------------------

Post by Todd
-----------------------------------------------------------
http://reviewboard.kde.org/r/3452/
-----------------------------------------------------------
(Updated 2010-03-31 02:56:41)
Review request for Gwenview.
Summary
-------
Currently in gwenview in order to disable or disable "Enlarge to fit" you must go to the gwenview configuration screen and enable or disable a checkbox. This patch adds an additional, much quicker method to accomplish the same goal. Pressing and holding on the "Fit" button in the statusbar now brings up a menu that allows you to switch between the two modes. The setting is remembered between sessions and is the same as the one in the configuration dialog, so changing it in one place also changes it in the other.
Diffs
-----
/trunk/KDE/kdegraphics/gwenview/lib/documentview/abstractdocumentviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/imageview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.cpp 1109333
Diff: http://reviewboard.kde.org/r/3452/diff
Testing
-------
Thanks,
Todd

Todd

2010-03-31 22:31:06 UTC

Permalink

Post by Todd

-1 from me.
This method is non-standard, sort of custom hack. all settings would be modified ONLY configration option from menu.

If you right click on the Dolphin address bar you can change several options that are also available from the configuration dialog. Those changes are lost when you start a new session, but if that is your primary criticism I could fix it with a one-line change to this patch. I was uncertain whether remembering the change across sessions was better or not, but I forgot to mention that in the description (although I did mention it in an earlier email). I could also make this so it is activated on right-click but that would be a more significant change.

- Todd

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4824
-----------------------------------------------------------

Todd

2010-03-31 22:57:29 UTC

Permalink

Post by Todd

-1 from me.
This method is non-standard, sort of custom hack. all settings would be modified ONLY configration option from menu.

If you right click on the Dolphin address bar you can change several options that are also available from the configuration dialog. Those changes are lost when you start a new session, but if that is your primary criticism I could fix it with a one-line change to this patch. I was uncertain whether remembering the change across sessions was better or not, but I forgot to mention that in the description (although I did mention it in an earlier email). I could also make this so it is activated on right-click but that would be a more significant change.
My concern is on usability on this, how useful this option will be for Gwenview's Target user group.Because this "menu" to enable or disable "Enlarge to fit" option is like a easter egg which is hidden from user. I am pretty certain, most users won't even know about it. unless they start clicking on every button or holding up for few secs to find out if some magical menu appears.

I agree, which is why it needs the little triangular indicator that most buttons with that capability have. Unfortunately I do not know how to implement that. It is supposed to appear automatically but isn't.

- Todd

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4824
-----------------------------------------------------------

Aurélien Gâteau

2010-03-31 21:16:47 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4822
-----------------------------------------------------------

Thanks for posting it on reviewboard.

I just gave it a try, and I must confess I am not really fond of it: it feels weird to have to hold the "Fit" button to get the menu. I don't expect users to find out about this. Maybe we need a separate "Config" button to the left of the "Fit" button?

- Aurélien

Aurélien Gâteau

2010-04-01 21:37:02 UTC

Permalink

Post by Todd

I don't like the idea of having both a "Shrink" (aka "fit") and a "Enlarge or Shrink" buttons: they are very similar and will only result in confusing users. In the end, maybe it's unfortunately better to stick to the existing code and only allow toggling of the "Enlarge to fit" option from the configuration dialog.

Maybe we can find a better solution if you can express more precisely in which situation you think it would be better to show images enlarged and in which situation it does not make sense.

- Aurélien

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4822
-----------------------------------------------------------

Todd

2010-04-01 22:18:04 UTC

Permalink

Post by Todd

It would be more obvious if there were a little arrow (or actually a triangle) like there is on most buttons that you can hold down to bring up a menu. It is supposed to appear automatically, but didn't (probably due to the subclassing). If it could be added to the button manually I think that would be the best solution, but I don't know how to do that.
If we are going to add another button, I think it would be better to go with my original suggestion and have a separate button that would enlarge or shrink. Then people wouldn't need to worry about configuring and the configuration option could be removed entirely.
I don't like the idea of having both a "Shrink" (aka "fit") and a "Enlarge or Shrink" buttons: they are very similar and will only result in confusing users. In the end, maybe it's unfortunately better to stick to the existing code and only allow toggling of the "Enlarge to fit" option from the configuration dialog.
Maybe we can find a better solution if you can express more precisely in which situation you think it would be better to show images enlarged and in which situation it does not make sense.

Would it be acceptable to have the dropdown indicator arrow? This is the standard Qt way to indicate that a toolbar button has a menu associated with it, and I have figured out how to add it. I am currently just working on getting the positioning right.

As for use cases, it really boils down to a trade-off between image clarity and image smoothness. In some situations having a large version of the image is more important (so you can see smaller details, or see it from further away), in other cases having a smoother image is more important. If you want the image smooth, then you wouldn't want to enlarge it. If you want the image large, then you do. It also depends somewhat in the size of the image, enlarging an image that is closer to the size of your screen will produce a more reasonable image than enlarging one that is not. That may, in fact, be the primary use case, enlarging images that are still reasonably clear when enlarged, while not enlarging images that aren't. But this cannot be done automatically, since it depends on things lings screen size, distance from the screen, image format, image quality, and a large amount (I suspect) of personal variability. But the current situation makes it infeasible to switch between the two cases routinely.

- Todd

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4822
-----------------------------------------------------------

Todd

2010-04-02 02:34:53 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/
-----------------------------------------------------------

(Updated 2010-04-02 02:34:53.143742)

Review request for Gwenview.

Changes
-------

I've added the standard Qt triangular indicator to show that the button has a menu associated with it. This makes the button's appearance consistent with other Qt and KDE toolbar buttons with similar menus.

Summary
-------

Currently in gwenview in order to disable or disable "Enlarge to fit" you must go to the gwenview configuration screen and enable or disable a checkbox. This patch adds an additional, much quicker method to accomplish the same goal. Pressing and holding on the "Fit" button in the statusbar now brings up a menu that allows you to switch between the two modes. The setting is remembered between sessions and is the same as the one in the configuration dialog, so changing it in one place also changes it in the other.

Diffs (updated)
-----

/trunk/KDE/kdegraphics/gwenview/lib/documentview/abstractdocumentviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/imageview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/statusbartoolbutton.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/statusbartoolbutton.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.cpp 1109333

Diff: http://reviewboard.kde.org/r/3452/diff

Testing
-------

Thanks,

Todd

Aurélien Gâteau

2010-04-04 22:01:47 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4875
-----------------------------------------------------------

I just tried it, and I still think it is not a good idea for users. I feel sorry for you because I can see how you spent quite some time on this, but the zoom slider area is already too crowded to my taste, and I think this feature won't be useful to enough people to justify making the zoom buttons wider and more cluttered.

An alternative which would avoid clutter would be to make the maximum zoom of "zoom to fit" mode configurable:
Right now "zoom to fit" restricts zoom between 0 and 100%. If an option was added to let the user specify that zoom to fit" is allowed to zoom up to, say, 200%, then images which almost fit the screen would be scaled to fit the screen, but smaller images wouldn't be zoomed more than 200% (or maybe they should stay at 100% in this case, but that would make the behavior when resizing weird). What is nice about this approach is you should get the zoom you expected without switching the way "zoom to fit" works, assuming the maximum zoom value is defined correctly.

- Aurélien

Post by Todd
-----------------------------------------------------------
http://reviewboard.kde.org/r/3452/
-----------------------------------------------------------
(Updated 2010-04-02 02:34:53)
Review request for Gwenview.
Summary
-------
Currently in gwenview in order to disable or disable "Enlarge to fit" you must go to the gwenview configuration screen and enable or disable a checkbox. This patch adds an additional, much quicker method to accomplish the same goal. Pressing and holding on the "Fit" button in the statusbar now brings up a menu that allows you to switch between the two modes. The setting is remembered between sessions and is the same as the one in the configuration dialog, so changing it in one place also changes it in the other.
Diffs
-----
/trunk/KDE/kdegraphics/gwenview/lib/documentview/abstractdocumentviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/imageview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/statusbartoolbutton.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/statusbartoolbutton.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.cpp 1109333
Diff: http://reviewboard.kde.org/r/3452/diff
Testing
-------
Thanks,
Todd

Todd

2010-04-10 18:36:25 UTC

Permalink

Post by Todd

Post by AurÃ©lien GÃ¢teau
I just tried it, and I still think it is not a good idea for users. I feel sorry for you because I can see how you spent quite some time on this, but the zoom slider area is already too crowded to my taste, and I think this feature won't be useful to enough people to justify making the zoom buttons wider and more cluttered.
Right now "zoom to fit" restricts zoom between 0 and 100%. If an option was added to let the user specify that zoom to fit" is allowed to zoom up to, say, 200%, then images which almost fit the screen would be scaled to fit the screen, but smaller images wouldn't be zoomed more than 200% (or maybe they should stay at 100% in this case, but that would make the behavior when resizing weird). What is nice about this approach is you should get the zoom you expected without switching the way "zoom to fit" works, assuming the maximum zoom value is defined correctly.

I guess that would work, if you use the zoom slider to set the maximum zoom. However, that means that "fit" and "100%" are no longer mutually exclusive. That would make me think that the two buttons should be separated into independent buttons. Implementing this is probably beyond my ability, though.

- Todd

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3452/#review4875
-----------------------------------------------------------

Post by Todd
-----------------------------------------------------------
http://reviewboard.kde.org/r/3452/
-----------------------------------------------------------
(Updated 2010-04-02 02:34:53)
Review request for Gwenview.
Summary
-------
Currently in gwenview in order to disable or disable "Enlarge to fit" you must go to the gwenview configuration screen and enable or disable a checkbox. This patch adds an additional, much quicker method to accomplish the same goal. Pressing and holding on the "Fit" button in the statusbar now brings up a menu that allows you to switch between the two modes. The setting is remembered between sessions and is the same as the one in the configuration dialog, so changing it in one place also changes it in the other.
Diffs
-----
/trunk/KDE/kdegraphics/gwenview/lib/documentview/abstractdocumentviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/documentview.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/documentview/imageviewadapter.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/imageview.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/statusbartoolbutton.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/statusbartoolbutton.cpp 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.h 1109333
/trunk/KDE/kdegraphics/gwenview/lib/zoomwidget.cpp 1109333
Diff: http://reviewboard.kde.org/r/3452/diff
Testing
-------
Thanks,
Todd