Landscape option added to base class

[jodros]

I missed an option for a landscape mode so here it is.

[Omikhleia]

Hello, that's something I considered too.

If it get accepted, it would need to be documented in the manual too. Not that we already have a PR on similar areas (#1853), so things will have to be done orderly.

Two small remarks in passing

• In general, I believe it's best, when possible, to first open an enhancement issue before a PR (to get discussions before heading toward an implementation)
• I see you merged master in your branch. Personally I would tend to recommend rebasing rather merging branches. It makes it easier when we change the target branch (e.g. to develop or other)

[jodros]

Hello @Omikhleia , thanks for your reviewing and I appreciate your tips.

This PR touches several lines unrelated to your change (adding or removing spaces, changing indentation....) -- I think this should be avoided (or done separately when wanted, but I don't think it's the case here), notably;

I have a habit to always use automatic formatting, so some of these indentation changes were made so. I'll pay attention to this detail next times.

In general, I believe it's best, when possible, to first open an enhancement issue before a PR (to get discussions before heading toward an implementation)

Right, I'll remember that.

[Omikhleia]

This will conflict with #1853 which introduces a sheetsize in addition to the papersize -- and in the grand scheme of things, possibly, both could be distinct with respect to using landscape or not. We'd need to discuss it.

[alerque]

As already noted, the unrelated style changes need to be removed from this PR. I can barely make out what is going on, much less give it a good review. I'm actually quite happy to accept code style fixes, but only on their own not as part of other changes. Also many of these changes go against our usual coding styles and won't fly anyway.

Adding a landscape option is fine but we do need it to be clean.

[alerque]

Also have we thought about whether an enum option like this (two named options) is best or whether an orientation option that could allow more rotations (e.g. 4) would be more robust?

[alerque]

Thanks for working with us to get this cleaned up. It's a simple option but it's worth implementing right so it is robust for future users.

Just a few nitpicks at this point mostly about boolean handling but there is one other logic issue I see.

[alerque]

@Omikhleia I'm pretty sure we can make a small adjustment and make this play nice with your PR for full bleed printing, but thinking about that does bring up a concern here. Are we going to have a problem with race conditions and the order (or Lua's non-deterministic order) of handling options?

[Omikhleia]

@Omikhleia I'm pretty sure we can make a small adjustment and make this play nice with your PR for full bleed printing, but thinking about that does bring up a concern here. Are we going to have a problem with race conditions and the order (or Lua's non-deterministic order) of handling options?

My remark #1892 (comment) got closed, and the current implementation uses some workaround do avoid it. It does have code smell (or adds up to the existing one, regarding how class options are implemented). What else can I say?

[alerque]

I just rewrote this and even changed the way the existing papersize class option is handled. This should avoid the race condition, but also reduces the code smell rather than adding too it. Reduces not eliminates. Again we all know the crop marks package here needs replacing entirely. I did fix the landscape option so it can be set on custom papersizes not just presets, added a unit test for it, etc.

@jodros You might look over the contents of my commits for future reference. Some of the changes (like not handling the papersize option twice!) are fixes to bugs that existed before your PR, but others are fixes for things you didn't handle or not setting random variables you don't use, etc. No worries, just some pointers for the next go-round...