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

wxGUI/history: Fix date formatting #3732

Merged

Conversation

lindakarlovska
Copy link
Contributor

Fixes #3726 by removing -d (it's linux only).

@lindakarlovska lindakarlovska added bug Something isn't working GUI wxGUI related labels May 24, 2024
@lindakarlovska lindakarlovska added this to the 8.4.1 milestone May 24, 2024
@lindakarlovska lindakarlovska self-assigned this May 24, 2024
@github-actions github-actions bot added the Python Related code is in Python label May 24, 2024
@lindakarlovska
Copy link
Contributor Author

Fixes #3726 by removing -d (it's linux only)

Implemented based on this comment (#3726 (comment)). Just to be sure, I tested it again on Windows and it is allright now..

@lindakarlovska lindakarlovska force-pushed the wxGUI-history-log-date-formatting-bug branch from 059619a to 40b44ae Compare May 24, 2024 07:17
@lindakarlovska lindakarlovska enabled auto-merge (squash) May 24, 2024 07:21
@echoix echoix modified the milestones: 8.4.1, 8.4.0 May 24, 2024
@echoix echoix added the windows Microsoft Windows specific label May 24, 2024
echoix
echoix previously approved these changes May 24, 2024
@echoix
Copy link
Member

echoix commented May 24, 2024

I was just wondering what the minus d in the old format strings meant. As from what I read, I would expect the B specifier to work on windows.

@lindakarlovska lindakarlovska removed the request for review from petrasovaa May 24, 2024 12:15
@lindakarlovska
Copy link
Contributor Author

I was just wondering what the minus d in the old format strings meant. As from what I read, I would expect the B specifier to work on windows.

Yes, the B specifier works on Windows and I am using it in month_name = day.strftime("%B"), the problem was only with -d

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{0} {1} makes it difficult to translate and it is not a good practice overall. If you are using the individual date components separately, use {month_name} {day_number} or something like that and then keyword arguments with the format call.

@lindakarlovska
Copy link
Contributor Author

{0} {1} makes it difficult to translate and it is not a good practice overall. If you are using the individual date components separately, use {month_name} {day_number} or something like that and then keyword arguments with the format call.

Ok, now it is more readable.

wenzeslaus
wenzeslaus previously approved these changes May 24, 2024
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just consider if preparing {month_name} {day_number} ahead of time and then using it later would make the code shorter and any changes easier (in translation or in the code itself).

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this needs to be wrapped in _() and the suffix makes it confusing and less flexible for the translator. The previous commit had all that right, just with duplication.

@lindakarlovska lindakarlovska force-pushed the wxGUI-history-log-date-formatting-bug branch from c799389 to cfed601 Compare May 24, 2024 14:56
echoix
echoix previously approved these changes May 25, 2024
@echoix
Copy link
Member

echoix commented May 25, 2024

I thought that this PR already gone through, I was set up to test the nightly again..

But it now has a different stack trace message (in windows sandbox), even if this fix isn't in there

@lindakarlovska
Copy link
Contributor Author

I thought that this PR already gone through, I was set up to test the nightly again..

But it now has a different stack trace message (in windows sandbox), even if this fix isn't in there

Thanks for reporting. That is strange that nobody has reported that earlier... It has nothing to do with the History tab. I am not near the computer now, but it looks like a problem in the main statusbar class which I implemented about one year ago... so we can calmly merge this PR, this issue should be solved separately.

@echoix
Copy link
Member

echoix commented May 25, 2024

I thought that this PR already gone through, I was set up to test the nightly again..
But it now has a different stack trace message (in windows sandbox), even if this fix isn't in there

Thanks for reporting. That is strange that nobody has reported that earlier... It has nothing to do with the History tab. I am not near the computer now, but it looks like a problem in the main statusbar class which I implemented about one year ago... so we can calmly merge this PR, this issue should be solved separately.

I agree, that's why I filed another issue for it. We're only waiting for Vaclav that previously requested changes, to change his review that unlocks it (or we can dismiss it if ever it gets stuck for too long, but we can still wait).

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It lost some of the good part from the previous version. Hopefully, now I'm clear enough on what I think should happen. Let me know if not.

gui/wxpython/history/tree.py Outdated Show resolved Hide resolved
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to request changes.

@lindakarlovska lindakarlovska merged commit f3ccff6 into OSGeo:main May 27, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working GUI wxGUI related Python Related code is in Python windows Microsoft Windows specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Windows GUI can't launch for 8.4.0dev (nightly)
4 participants