Skip to content
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

Change Planet Suitiability alignment to spaces, add logging #2202

Merged
merged 4 commits into from Jul 13, 2018

Conversation

Projects
5 participants
@dbenage-cx
Copy link
Member

commented Jul 4, 2018

Attempt to address #1936 with some added logging for future troubleshooting.

(Edit to prevent auto-close of issue)

@@ -2689,30 +2712,38 @@ namespace {

if (it->first > 0) {
if (!positive_header_placed) {
detailed_description += str(FlexibleFormat(UserString("ENC_SUITABILITY_REPORT_POSITIVE_HEADER"))
auto pos_header = str(FlexibleFormat(UserString("ENC_SUITABILITY_REPORT_POSITIVE_HEADER"))
% planet->PublicName(planet_id));

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 4, 2018

Member

fix indenting

This comment has been minimized.

Copy link
@dbenage-cx

dbenage-cx Jul 4, 2018

Author Member

Fixed

@dbenage-cx

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2018

The one remaining tab is in the stringtable entry ENC_SPECIES_PLANET_TYPE_SUITABILITY_COLUMN1

At least on my system, this tab is needed to align the text to a width divisible by the width of a space character. (e.g. a width of 10 vs a space width of 4)
Otherwise some text ends up with a slight offset less than the width of a space.
I left this in stringtables for easier troubleshooting if it is an issue on other platforms.

(Edit: Tab now removed, leaving comment)

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

Interacts weirdly with drag-selecting text, but that's probably not an issue specific these changes...
drag_select_layout_weirdness

@dbenage-cx

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2018

Just verified that behavior is consistent with release-v0.4.8 branch.

Removing the above mentioned tab prevents such behavior, marking as WIP to try some alternatives.

@Oberlus

This comment has been minimized.

Copy link

commented Jul 4, 2018

Interacts weirdly with drag-selecting text, but that's probably not an issue specific these changes...

With tabs it also interacts weirdly with drag-selection.

@dbenage-cx dbenage-cx force-pushed the dbenage-cx:ref-1936_log_suitability branch from d747954 to f398139 Jul 4, 2018

@dbenage-cx

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2018

Workaround to selection done with use of hair space (0x200A) to buffer the shorter entry width.
This was the only whitespace character I found to resolve with an extent width less than 4.

User selection of text containing a tab is still potentially an issue in other entries/places.

@dbenage-cx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

This PR is currently crashing on Win10 display of report with Invalid UTF8, but does not crash 100% of time

@dbenage-cx dbenage-cx force-pushed the dbenage-cx:ref-1936_log_suitability branch from f398139 to 0199bd1 Jul 8, 2018

@dbenage-cx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

Rebased

@Vezzra

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

@dbenage-cx, do you still get those Invalid-UTF8 crashes on Windows, or does the removal of the "work in progress" label mean you've got that resolved?

I'm asking because this PR can only be cherry picked to the release branch if it doesn't introduce crashes.

@dbenage-cx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

@Vezzra Yes, they were resolved by storing u8 into std::string instead of passing u8.

@Dilvish-fo

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

It looks like it was a unittest failing here, triggered by an AI client exception, which seems I've seen before recently and certainly is unrelated to this PR:

02:35:51.841100 [error] python : configure_logging.py:197 : Uncaught exception: Traceback (most recent call last):
02:35:51.841132 [error] python : configure_logging.py:197 :   File "/freeorion/default/python/AI/FreeOrionAI.py", line 382, in <module>
02:35:51.841144 [error] python : configure_logging.py:197 :     init_handlers(fo.getOptionsDBOptionStr("ai-config"), fo.getAIDir())
02:35:51.841151 [error] python : configure_logging.py:197 :   File "/freeorion/default/python/AI/freeorion_tools/handlers.py", line 10, in init_handlers
02:35:51.841158 [error] python : configure_logging.py:197 :     handlers = split(get_option_dict()[HANDLERS])
02:35:51.841164 [error] python : configure_logging.py:197 : KeyError: 'handlers'
02:35:51.841170 [error] python : configure_logging.py:197 : 

So I don't think that should hold up this PR. But I expect that was a weird flukey failure (which we really should try to figure out better why it's periodically happening), so I'll see if I can finally figure out how to restart the Travis CI tests.

@Dilvish-fo

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

OK, well at least I finally figured out why I wasn't seeing the restart option before-- I had to log into https://travis-ci.org/profile in order for it to sync up with my github account.

@Dilvish-fo Dilvish-fo merged commit 41cdbde into freeorion:master Jul 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Dilvish-fo Dilvish-fo moved this from Proposed to Accepted in 0.4.8 Release Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.