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

Use Yast::Builtins.strftime in 'Change Filter' dialog #45

Merged
merged 6 commits into from
Sep 27, 2022

Conversation

srinidhibs
Copy link
Contributor

@srinidhibs srinidhibs commented Dec 15, 2019

Fixes bsc#1081459
"SLEx_15_LOC : YaST:ALL_LANGS:Possible untranslated day abbreviation in Systemd Journal/Entries to Display Filter dialog"

Steps to Reproduce:

  1. Launch YaST
  2. Go to 'Miscellaneous' section
  3. Click on Systemd Journal icon
  4. Journal appears
    Click 'Change Filter'
  5. 'Entries to Display' dialog appears
    Check 'Since systems's boot..' text string for defects

Actual Results:

'Sun' appears as the abbreviation for Sunday in the string for German
Is this correct?

@coveralls
Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage increased (+0.4%) to 84.987% when pulling a94a14b on srinidhibs:bnc#1081459 into cce5aad on yast:master.

@@ -96,6 +96,11 @@ def interval_description
end
end

# Split journalctl timestamp at "—"; localize and return the joined string back
def self.localize_timestamp(ts)
ts.map { |t| Yast::Builtins.strftime(t, "%c") }.join(" to ")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a non-translatable string. No matter in which language you are, it will always be
"$DATE1 to $DATE2". The dates themselves maybe look nicer than the previous version (which uses the default journalctl output which is not exactly pretty), but we should not introduce a hardcoded English sentence.

@dgdavid
Copy link
Member

dgdavid commented Jul 28, 2021

Hi @srinidhibs!

I hope you’re staying healthy.

Do you plan to continue with this PR?

Thanks!

@srinidhibs
Copy link
Contributor Author

Hello @dgdavid

I hope you’re staying healthy.

Yes, thank you for asking! I hope the same for you as well!

Do you plan to continue with this PR?

Yes. I'm really sorry about the radio silence! The machine which I was using for my FOSS contributions had run into some issues. I'll work on it this weekend and try to update this PR soon after that.

Regards,
Srinidhi.


# TRANSLATORS: %<first>s and %<last>s will be replaced by already localized
# timestamps indicating when the system booted and when it was shut down
_(format("%<first>s to %<last>s", first: start_time, last: end_time))
Copy link
Contributor

Choose a reason for hiding this comment

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

This cannot work, you are executing format first and then passing the result (for example "2021-03-20 to 2021-03-21") to _(). Obviously, such string will not be found for translation.

It can only work the other way around: format(_("blah %<first>"), first: start_time)

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you apply some format to the timestamps here? Right now it would look like this (in my current Ruby version, that may change in the future):

2021-08-27 10:31:01 +0200 to 2021-08-30 15:36:09 +0200

I think that looks even uglier than before.

What about something like this?

def self.localize_timestamp(boot)
  start_time = Yast::Builtins.strftime(boot[:timestamps][:first], "%c")
  end_time = Yast::Builtins.strftime(boot[:timestamps][:last], "%c")

 # TRANSLATORS: blah
 format(_("%<first>s to %<last>s"), first: start_time, last: end_time)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ancorgs I'm sorry! There are a couple of changes that I forgot to push. I thought of using the existing TIME_FORMAT that is defined in the same file:

TIME_FORMAT = "%b %d %H:%M:%S".freeze

Would this be acceptable? I've also fixed the format(_(...)) problem that you called out earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it fully makes sense.

Again, sorry for the late reply. :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

Those extra changes were never pushed. But it's a shame to leave this contribution in this almost-finished status. So I guess I will cherry-pick @srinidhibs commits in a new branch and try to fix the standing issues myself.

Unless @srinidhibs still has those commits around and he manages to finally push them, of course. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ancorgs! I'm sorry for dropping the ball on this one! Can you please give me a couple of days? I'll try to find out if I've unfinished work in my local branch and try to close this soon.

mvidner and others added 3 commits September 26, 2022 12:07
from QueryPresenter#interval_description

enabled by making the `boot[:timestamps]` hash mimic the `interval` hash
@mvidner mvidner merged commit a204dea into yast:master Sep 27, 2022
@yast-bot
Copy link

✔️ Internal Jenkins job #18 successfully finished
✔️ Created IBS submit request #280841

@yast-bot
Copy link

✔️ Public Jenkins job #27 successfully finished
✔️ Created OBS submit request #1006340

mvidner added a commit that referenced this pull request Oct 4, 2022
PR #45 reintroduced a dependency on the old output of
`journalctl --list-boots` by looking for an Em Dash to separate the two
timestamps.

Now there is a space. Let's allow the Em Dash for compatibility.
test/data/list-boots-*.out use both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants