New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model print enhancements - General settings #5568

Merged
merged 2 commits into from Dec 29, 2017

Conversation

Projects
None yet
2 participants
@elecpower
Copy link
Contributor

elecpower commented Dec 26, 2017

Next batch of enhancements.
Tested using X9D+ settings.
Some formatting compromises to support model comparison.
Telemetry next
X12 will need extra fields printed and additional values specific to radio
@mpaperno will come back and refactor fieldToString and 'magic'/raw throughout Companion once all the fields are printed
Partly fixes #5462

@mpaperno

This comment has been minimized.

Copy link
Member

mpaperno commented Dec 28, 2017

Looks great! Tested with several models, didn't see any issues. 👍

My only comment/request so far is to make the default text color black instead of gray... nothing really to do with this PR, I know. But it's hard to read. I think the headings/labels being bold will be enough of a distinction. I believe the default text color is set in multimodelprinter.h:54 as the default argument value to endCompare() (was a bit hard to find).

I guess the other minor thing is how in a lot of cases the item details are appended in parenthesis but no space. (Not a criticism of this PR as I understand it is just following current convention.) E.g.:

1: Internal Radio System, Protocol(FrSky XJT (D16)), Channels(1-16), Receiver(0)

I understand this is/was desirable in the Inputs/Mixes editor to save space (which uses that part of ModelPrinter), but in the other cases it seems unnecessary. For example I'd propose:

1: Internal Radio System; Protocol: FrSky XJT (D16); Channels: 1-16; Receiver: 0;

I don't want to get into a formatting debate, but I do wonder if having a shared helper function to format those parts would be useful. e.g.:

QString formatItem(const QString & name, const QString & value) {
  return QString("%1(%2); ").arg(name, value);
}

Anyway, great work, thanks!

@elecpower

This comment has been minimized.

Copy link
Contributor Author

elecpower commented Dec 29, 2017

Agree with you @mpaperno on formatting inconsistencies and these need attention. Agree with some Helper functions / macros as lots of repeat formatting. Also took into account your recent translation refactoring. There is conflicted formatting when values used for model comparison (red, green, grey, black, regular, bold).
I toyed with various layouts and whilst they worked for a single model they often resulted in corrupt formatting when used for model comparisons so took the 'safer' approach when this occurred.
With the devs working so fast on lots of changes and me plodding along, my strategy was to print out the values with some structure as something is better than nothing. Then have a second pass review the various layouts used and tune it up based on feedback like yours.
Also I'm probably going to create separate print for Inputs and Mixes to avoid breaking the lists. IMHO the lists should not be using print functions.

@mpaperno

This comment has been minimized.

Copy link
Member

mpaperno commented Dec 29, 2017

That all sounds good to me and makes sense. Do you want to work more on this PR, or merge it now and refine later?

@elecpower

This comment has been minimized.

Copy link
Contributor Author

elecpower commented Dec 29, 2017

I would prefer to merge as is and revisit once I have finished the first pass. This will also provide others the opportunity to find any bugs in my reverse engineering of the radio variants which I can address in the next pass.
I have already finished the failsafes and basic telemetry. Working on telemetry sensors (these will be 'fun' to format)

@mpaperno mpaperno merged commit 96d9ae4 into opentx:2.2 Dec 29, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@mpaperno

This comment has been minimized.

Copy link
Member

mpaperno commented Dec 29, 2017

Thanks!

@elecpower elecpower deleted the elecpower:Model_Print_Enh branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment