Use FlowLayout for Crop toolbar
ClosedPublic

Authored by muhlenpfordt on Sep 10 2018, 8:38 AM.

Details

Summary

The Crop toolbar with enabled Advanced settings is very wide
and it's not possible to make the Gwenview window smaller with this
toolbar visible.
Using a FlowLayout for the Crop toolbar avoids this issue and breaks
the bar into multiple lines if there is not enough horizontal space.

Before:

After:

Test Plan
  • Check Crop toolbar behaviour while resizing Gwenview window
  • Check Crop toolbar with Advanced settings en-/disabled
  • Check RedEye toolbar (uses the same SlideContainer)
  • Check SaveBar after modifying images (uses SlideContainer)
  • Check Browse Mode Add Filter (uses FlowLayout)

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt created this revision.Sep 10 2018, 8:38 AM
Restricted Application added a project: Gwenview. · View Herald TranscriptSep 10 2018, 8:38 AM
muhlenpfordt requested review of this revision.Sep 10 2018, 8:38 AM

I found no way to add a custom layout to Qt Designer, so CropWidget is now created in (not generated) code.

lib/documentview/documentviewcontroller.cpp
43–69

This widget has to be removed because it prevents the FlowLayout from resizing and using the whole width. As a result of this removal the RedEyeReductionWidget has to be modified too to retain color and centering.

Thanks, this looks like it'll be a nice improvement! I'll review soon.

muhlenpfordt edited the summary of this revision. (Show Details)

Center all lines in multiline layout too
Readded original margin (Crop and RedEye toolbar)

Thanks, this looks like it'll be a nice improvement! I'll review soon.

Hope you did not started yet before I updated and the centering/margin version looks better now. :)

Behavior seems perfect to me! Code review is not my strong suit but I'll give it a shot.

Set background in tool container (fixes slide out color change)
Use for instead of foreach loops

Sorry it's taken me so long to get to this. One thing I notice is the paradigm of putting layouts inside their own widgets. Generally I don't think this is necessary. Also, widgets that will become a part of a layout don't need to be given a parent; Qt automatically re-parents them to the layout you add them to, so the parent argument can simply be left out.

Sorry it's taken me so long to get to this. One thing I notice is the paradigm of putting layouts inside their own widgets. Generally I don't think this is necessary.

No problem, we are in no hurry. ;)
I'm not sure what you mean with 'layouts inside their own widgets'. The box widgets are needed to keep some items together (e.g. two spinboxes and their label).

Also, widgets that will become a part of a layout don't need to be given a parent; Qt automatically re-parents them to the layout you add them to, so the parent argument can simply be left out.

I mostly used the generated code from Qt Designer as template. In fact Qt sets a parent but this does not always seem the one we need.
If I don't set the parent in boxWidget(QWidget* parent) this results in strange layout effects.
If I don't set the box as parent for e.g. the spinboxes they are not visible.

Moved vertical item centering from CropWidget to FlowLayout
Removed some superfluous layout settings
Some style fixes

ngraham accepted this revision.Dec 17 2018, 12:44 AM

Sorry for taking so long on this! Feel free to land at any time.

This revision is now accepted and ready to land.Dec 17 2018, 12:44 AM

I found no way to add a custom layout to Qt Designer

Stupid question, may this help? https://doc.qt.io/qt-5/designer-using-custom-widgets.html
It's about widgets but perhaps it works with layouts too

QWidget* mFooWidget;

I prefer this style too, but KDE coding style says to use "QWidget *mFooWidget;" (*& always to the right)

Yes, it's not a support forum, but may I ask if it's (currently) possible to place these tool bar at the top? If not it would be nice to have this possibility. Furthermore looks there Widgets/Texts here always a little smaller than usually (?)

shubham added a subscriber: shubham.EditedDec 17 2018, 5:36 AM

I prefer this style too, but KDE coding style says to use "QWidget *mFooWidget;" (*& always to the right)

It is not strict to conform to it, the basic formula is to see the surrounding code and follow the same coding style.

Landing on Peter's behalf since he's probably on vacation right now. :)

This revision was automatically updated to reflect the committed changes.

Landing on Peter's behalf since he's probably on vacation right now. :)

Thanks! I'm a bit busy at the moment - hope I'll find more time again in a few weeks.