KScreenGenie moved to KDE Review

[Boudhayan Gupta]

Hi Thomas,

On 29 June 2015 at 03:44, Thomas Lübking <thomas.luebking at gmail.com> wrote:
> Did a functional test only.
>
> a) The button role assignment seems wrong (you tried to arrange buttons in a
> certain order that suits either the KDE or windows order? They end up
> totally weird other orders eg. GTK or OSX)
>

It's a standard QDialogButtonBox. I haven't set the button ordering
manually at all. Maybe it's a QDialogButtonBox bug then.

>
> b) Buttons
>   1. While I like the "Save & Exit" idea, it leaves uncertainity about what
> happens. Eg. there could be a tooltip that says *where* the file is saved.
> Bonus points for allowing to set a pattern (location + filename)

A tooltip is already set that says "Quicksave screenshot in your
Pictures directory and exit".

>   2. There're two "Save" buttons, "Save As..." could maybe be moved into a
> popup of "Save & Exit" (which could also access a dialog to set the
> quicksave pattern)

This is planned.

>   3. There's no way for direct mass shooting (ie. "Save & Exit" minus
> "Exit") - some sort of "Set filename pattern for this session" feature ("Set
> pattern..." vs. "Set default pattern..."?) could turn "Save & Exit" into
> "Save"

I don't understand. "Direct mass shooting"?

>   4. No idea whether I miss some plugin, but "Send to..." is "Open with..."
> here (+copy to clipboard)

Same deal with KSnapshot, except for "Copy To Clipboard", which I
moved there to save a button.

>   5. "Discard" should turn "Exit" after the file is saved in any way (but
> not to clipboard, w/o klipper the file would be lost) since the image isn't
> discared at all.

This is a good idea. Will implement.

>   6. There's a "Print..." button but no "Copy to clipboard" button... ;-)
>      I don't yet know how to finally resolve this, but printing is rare and
> copy to clipboard is ... less rare, but it does not fit into "Send to", esp.
> not if that's actually "Open with"
>

In KSnapshot, the printing was handled by KIPI. Here I've got explicit
code to handle printing manually, and I've done stuff to make it
render the image at high DPI (which wasn't being done in KSnapshot).

Would you prefer I swap the Clipboard option in the Send To menu with
Print, and put Clipboard on the buttons?

>
> c) I agree with Hugo that "Active window" is totally weird. It captures the
> window that kwin passes the focus after kscreengenie closed, what depends on
> two kwin settings.... (if the WM is KWin)
>   The difference to ksnapshot is however not the behavior, but that
> ksnapshot doesn't have that feature in the GUI at all (since it doesn't make
> any sense - the active window is kscreengenie at the time of usage) -
> ksnapshot allows to capture the "Window under the mouse" which is actually
> "pick window" - kscreengenie even clicks the window KWin will automagically
> activate if using "On Click" what is totally confusing. At least in this
> mode, kscreengenie should definitively shoot the picked window, not the
> active one. In doubt by activating it before.
>

Alright, I'll revert to KSnapshot behaviour and relabel to Window
Under Mouse, but I cannot guarantee we can have the same behaviour in
Wayland.

>
> d) "Capture Area | Active Window" - *cough* ;-)
>   My suggestion would be to label the combobox "Mode" reverse the "On click"
> checkbox and label it "Delay" (omitting the extra label)
>

Is that much better than the current solution? IMO the way KSnapshot
merges delay and on-click into one spinbox is bad, there's no way to
set a zero-delay no-onclick mode. This is why I did it this way. If I
want to wait for a click, I want to say that explicitly.

>
> e) Technically "Capture Mode" and "Capture Options" are capture options -
> "Capture Options" are actually "Content Options" and delay/onclick are
> "Trigger options", though I don't know whether there needs to be a
> horizontal structure at all since
>
> f) because of the flat grouboxes (?) and the splitter of the buttongroup,
> the "Take New Screenshot" buttons looks like it belongs to the "Capture
> Options".
> Maybe combine all options, put a splitter below and the "Take New
> Screenshot" button below that? (So it's visually isolated between options
> and buttonbox)
>

I've made it so that the entire right hand side of the window is
dedicated to options and buttons, and the entire left hand side is
dedicated to the preview. Makes more sense given 90% of displays are
widescreen nowadays, and it's not so wide so as to be a nuisance on
the 4:3 displays.

>
> g) pressing "Escape" should abort the screenshot.
>

It already does.

>
> h) It's not like I could find it, but afair the rule is "Take new
> screenshot", not "Take New Screenshot" - at least in labels.
>
> i) A feature that kscreengenie imo lacks (like ksnapshot does) is some basic
> annotation features (ellipsis, arrow, labelbox) - I'd volunteer to write a
> patch for that ;-)
>

There's a mini-editor feature planned yes, but I wasn't aware
KSnapshot already does it. It must be a new addition, since it wasn't
there in the code two months ago.

>
> j) > I've found use in setting half-second delays many times.
> While I actually cannot think of a reason for this precision, the single
> step width should then be 0.5 seconds - otherwise it's nasty to change the
> value with the mouse wheel or the spinbox buttons.

If the usability team wants larger steps, I guess I have to do it. But
personally  I've set even 0.2 sec timeouts, although they may have
been for special reasons.

>
> Cheers,
> Thomas
>
> PS:
> +1 for keeping "ksnapshot" - it's familiar and simpler to vocalize. But
> ultimately I don't care =)

Thanks,
Boudhayan