issue/8/ksnapshot #54

This fixes issue #8 (first commit)

the second one fixes the indentation.

If you approve, I would squash them.

Hi Emanoil,
I haven't studied the code yet, but I see lot of changes that are only tabs-space changes and make reviewing the patch more complex.
Although fixing the indentation is a good thing in general, it is fixing it against the TDE code guideline (which is not yet really enforced), so I would suggest leaving all unneceddary unchanged lines untouch and get the commits to focus on the code changes.
Could you rework the commits in that way?

Hi Emanoil,
I haven't studied the code yet, but I see lot of changes that are only tabs-space changes and make reviewing the patch more complex.

I have splitted the commits in one that fixes the issue and another to correct the indentation
but now I removed the second, so have a look, please.

Although fixing the indentation is a good thing in general, it is fixing it against the TDE code guideline (which is not yet really enforced), so I would suggest leaving all unneceddary unchanged lines untouch and get the commits to focus on the code changes.

Actually I thought it is documented and I configured Eclipse based on the documentation, but anyway - we can discuss this in another thread.

Actually I thought it is documented and I configured Eclipse based on the documentation, but anyway - we can discuss this in another thread.

It is documented but not yet enforced, so usually we currently follow whatever format is in a file at the moment. There are still changes required in uncrustify (the formatting tool) before we can apply the code style automatically. I am slowly working on that task too :-)

Excellent fix!

I have splitted the commits in one that fixes the issue and another to correct the indentation
but now I removed the second, so have a look, please.

Wow, such a short and clean fix!
Clearly reviewing this last version is way quicker than reviewing the same thing mixed with indentation changes :-)

Can you rebase the commit on top of master before I merge? There was another commit in between and this PR is no longer of top.

And could you also slightly reword the commit message by adding "Issue #8" in place of "#8". Something like this:

KSnapshot: fix Print orientation in ksnapshot cannot be selected. This resolves issue #8.

This will help the script that prepares the detail commit log, as far as I am aware.

Once you edit the message in commit, do not forget to make rebase to the current head.

@deloptes
Thanks for rebasing, adjusting the commit and the fix :-)

After thinking about this for a while, I think this fix is not right. Although it allows to choose the printing orientation, it takes away the default selection and force the user to have to choose the orientation in most cases, since a lot of snapshot will probably use landscape mode.

Currently, setting the orientation in KPrinter has the side effect to lock the orientation to be fixed. I think a better fix would be to be able to set the preferred orientation without necessarily locking the selection.
I can prepare a PR for it if you ( @SlavekB, @blu.256, @deloptes ) agree with me. Btw, by memory I think there is a similar bug filed against kpdf, but I need to look it up.

Yes, the ability to cleverly set the default orientation without locking the possibility to change it by the user was the first thing I think about. So yes, I agree with this idea.

issue/8/ksnapshot #54

Εξεταστές