OcdFileExport: Fix export of custom tabulator positions

[dl3sdo]

When exporting more than 32 custom tabulator positions the num_tabs
field is now limited to the 32 tabs that are actually only exported and
a warning messages is issued (issue was detected by lpechacek).
In addition the export now converts the tabulator position to a 32 bit
value since the convertSize function that was used before converted it
to a 16 bit value instead (which was indeed only relevant for tab
values larger than 327,67mm).

[dl3sdo]

Please find attached an .omap example that contains 34 custom tabulator positions (of which some exceed 327,67mm).
ExportTextTabsSample_34TabsWithLargeValues.zip

[dl3sdo]

@lpechacek: may I ask you again for a review since you brought this issue up?

[lpechacek]

Apologies for the long comment.

Thanks, Matthias, for the patches. Regarding the type conversion, I took a longer look at it and I can see that it's not exactly an easy topic. Here are my observations and a hint about the fix.

1. There are multiple convert*() functions in the ocd_file_export code. Two of them convert directly to the target type (convertSize() and convertColor()). The last one (convertRotation()) produces an integer and the type conversion is done through a cast.
2. It is only the custom tabs positions where the type don't match. You can identify the spot by wrapping the assignment arguments into curly braces to make the compiler warn about narrowing conversions (sed -i 's/ = convert\(\(Color\|Size\|Rotation\).*\);/ = { convert\1 };/' src/fileformats/ocd_file_export.cpp).
3. We already have inconsistent approaches to the type selection in the code. While your patch aligns the structure between the import and export code, it does not address the inconsistency among the usage of the conversion functions.

I can see large room for improvement. Here are my proposals what we can do next:

P1) Just make a minimal fix (use type cast for the overload selection: convertSize(qint64(text_symbol->getCustomTab(i)));), possibly fix the tab_pos type (should be qint32). This addresses the immediate problem and can remain the only action here.

P2) Rework the code to be explicit about the conversion. I can see two options.

P2.1) Provide template functions for convert*() and select the return type on each value conversion spot. The usage would be as follows:
ocd_text_special.tab_pos[i] = convertSize<qint32>(text_symbol->getCustomTab(i));
and at another place
ocd_object.angle = convertRotation<decltype(ocd_object.angle)>(path->getPatternRotation());
The drawback is the necessity of duplicating the target variable name in many cases because the compiler cannot infer the function return type in the assignment, to my knowledge. The benefit is that the assignments stay as they are.

P2.2) Provide template functions with an output parameter for the conversion. The usage would be:
convertSize(text_symbol->getCustomTab(I), ocd_text_special.tab_pos[i]);
and
convertRotationpath->getPatternRotation(), ocd_object.angle);.
In this case, the compiler sees clearly the target type and there is little room for code-level inconsistency. The drawback is that the code might be more difficult to read without the explicit assignments.

With both P2.1 and P2.2 we can also perform run-time checks on the values and catch overflows.

My preference is towards P2.2, or doing P1+P2.2. While it may make the code look slightly weird, I believe that it will make it more robust. @dg0yt, @dl3sdo, opinions?

[lpechacek]

11d447b is a prototype implementation of P2.2. Maybe it's overkill but I hope that the patch at least triggers the discussion about which directions we should stop exploring.

[dg0yt]

I want to comment only briefly on the proposals:

• Focus this PR only on the fix of tab position export. This is Libor's "P1" IIUC.
• Don't turn free functions into (non-static) member function (unless needed externally).
• Don't add templates unless they really add value.
  In the header, they are expensive for every usage of that header (or even pointless if only declared).
• With regard to 11d447b:
  Don't turn pure functions into "functions with output parameters" unless needed.

convertSize came from the legacy implementation of OCAD format 8. I guess the signature were designed for the needs at that time.
What it does do right is to tag a certain type of conversion (unit of measurement). It also seems to do the right thing for most uses. There is nothing wrong with using explicit type conversions together with the function call in a few cases.

I would agree there is room for improvement. (At least they should go into an anonymous namespace, as an implementation detail internal to this translation unit.) But before jumping into templates functions, I would think about replacing the trivial integer types by struct types that signal that actual unit of measurement. Anything that helps the compiler to easily select the the proper conversion function. And there are not so many that we couldn't define them explicitly, without templates.

[dg0yt]

Focus this PR only on the fix of tab position export.

[lpechacek]

But before jumping into templates functions, I would think about replacing the trivial integer types by struct types that signal that actual unit of measurement. Anything that helps the compiler to easily select the proper conversion function. And there are not so many that we couldn't define them explicitly, without templates.

Thanks for the comment! I like the proposal. I'll record the conclusion in the refactoring project.

@dl3sdo, please implement a minimal change for the correct convertSize() overload selection as hinted in P1. Adjust the commit message accordingly, i.e. drop the part about class methods. Then I think the change will be complete. Thanks!

[dl3sdo]

Libor, Kai, thank you for your review and feedback. I fully agree to the proposal to just address and solve the current issue and to postpone a general rework.
We should keep in mind that there are other inconsistencies between .ocd types and their mapping to Mapper types (see #2045 (comment))