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

Ugrade hardcoded new language String #4889

Closed
wintstar opened this Issue Aug 2, 2018 · 15 comments

Comments

Projects
None yet
5 participants
@wintstar

wintstar commented Aug 2, 2018

Forum version: SMF 2.1 Beta 4 (more detailed)
Current SMF version: SMF 2.1 Beta 3
GD version: bundled (2.1.0 compatible)
MySQLi engine: MariaDB
MySQLi version: 10.1.29-MariaDB
Alternative PHP Cache: 5.1.2
PHP: 7.1.11 (more detailed)
Server version: Apache/2.4.33 (Win32) OpenSSL/1.0.2o PHP/7.1.11
Browser: Firefox Quantum 61.0.1 (64-bit)
Last Pull Request of Today 4873

I have update my german language strings, but one Text is not translatet:
Completed in

setInnerHTML(document.getElementById("upgradeCompleted"), "Completed in " + totalTime);';

upgrade_new_strings

The string seems to exist:
$txt['upgrade_completed_time2'] = 'Completed in %1$d';

@Gwenwyfar Gwenwyfar added the Language label Aug 2, 2018

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Aug 2, 2018

Member

I'm not able to test it now. Can you tell me if this works?

setInnerHTML(document.getElementById("upgradeCompleted"), "', sprintf($txt['upgrade_completed_time2'], totalTime), '");';

Member

frandominguez03 commented Aug 2, 2018

I'm not able to test it now. Can you tell me if this works?

setInnerHTML(document.getElementById("upgradeCompleted"), "', sprintf($txt['upgrade_completed_time2'], totalTime), '");';

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Aug 2, 2018

Text is no longer displayed, and the upgrade script stops at total progress 14% and progress 0.2%.

wintstar commented Aug 2, 2018

Text is no longer displayed, and the upgrade script stops at total progress 14% and progress 0.2%.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Aug 3, 2018

Member

Actually this would be harder to achieve. If you change the line to:

setInnerHTML(document.getElementById("upgradeCompleted"), "', $txt['sample'], '" + totalTime);';

You'll get the first part localized, so translator may translate "Completed in" but not totalTime, because of this:

SMF2.1/other/upgrade.php

Lines 4305 to 4311 in 1c20efc

var totalTime = "";
if (diffHours > 0)
totalTime = totalTime + diffHours + " hour" + (diffHours > 1 ? "s" : "") + " ";
if (diffMinutes > 0)
totalTime = totalTime + diffMinutes + " minute" + (diffMinutes > 1 ? "s" : "") + " ";
if (diffSeconds > 0)
totalTime = totalTime + diffSeconds + " second" + (diffSeconds > 1 ? "s" : "");

We could translate "hour", "minute" and "second" but they add an aditional s at the end if the variable is higher than 1, so I don't know what would be the best approach here. Thoughts?

Member

frandominguez03 commented Aug 3, 2018

Actually this would be harder to achieve. If you change the line to:

setInnerHTML(document.getElementById("upgradeCompleted"), "', $txt['sample'], '" + totalTime);';

You'll get the first part localized, so translator may translate "Completed in" but not totalTime, because of this:

SMF2.1/other/upgrade.php

Lines 4305 to 4311 in 1c20efc

var totalTime = "";
if (diffHours > 0)
totalTime = totalTime + diffHours + " hour" + (diffHours > 1 ? "s" : "") + " ";
if (diffMinutes > 0)
totalTime = totalTime + diffMinutes + " minute" + (diffMinutes > 1 ? "s" : "") + " ";
if (diffSeconds > 0)
totalTime = totalTime + diffSeconds + " second" + (diffSeconds > 1 ? "s" : "");

We could translate "hour", "minute" and "second" but they add an aditional s at the end if the variable is higher than 1, so I don't know what would be the best approach here. Thoughts?

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Aug 3, 2018

What would be added already exists. Have a look at the screenshot. I just noticed it myself.

Gesamtverarbeitungszeit (german) is the same as Completed in (english)

upgrade_new_strings2

wintstar commented Aug 3, 2018

What would be added already exists. Have a look at the screenshot. I just noticed it myself.

Gesamtverarbeitungszeit (german) is the same as Completed in (english)

upgrade_new_strings2

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Aug 3, 2018

Member

So leaving like that should work? Try adding a new text string that only contains "Completed in" in German and then place it in the line I pasted above. As I don't have any language packs installed I can't properly test it, so if you can say if that works, that'd be great.

Member

frandominguez03 commented Aug 3, 2018

So leaving like that should work? Try adding a new text string that only contains "Completed in" in German and then place it in the line I pasted above. As I don't have any language packs installed I can't properly test it, so if you can say if that works, that'd be great.

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Aug 3, 2018

setInnerHTML(document.getElementById("upgradeCompleted"), "', $txt['sample'], '" + totalTime);';

This can be removed completely, it already exists. See screenshot. The Completed in display already exists before Pull Request #4873. Look here:

', $txt['upgrade_time_elapsed'], ':

The same function.

wintstar commented Aug 3, 2018

setInnerHTML(document.getElementById("upgradeCompleted"), "', $txt['sample'], '" + totalTime);';

This can be removed completely, it already exists. See screenshot. The Completed in display already exists before Pull Request #4873. Look here:

', $txt['upgrade_time_elapsed'], ':

The same function.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Aug 3, 2018

Member

Actually both aren't the same text. One of them is "Time Elapsed" (the upper one), and the other one is "Completed in".

Also, "Completed in" is only shown if extra debug information option is enabled, afaik.

Member

frandominguez03 commented Aug 3, 2018

Actually both aren't the same text. One of them is "Time Elapsed" (the upper one), and the other one is "Completed in".

Also, "Completed in" is only shown if extra debug information option is enabled, afaik.

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Aug 3, 2018

Look at the screenshot above. The same time is displayed. It may be a different text but it has the same function.

wintstar commented Aug 3, 2018

Look at the screenshot above. The same time is displayed. It may be a different text but it has the same function.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Aug 3, 2018

Member

Yes, it has the same function and actually shouldn't, so we don't want to remove it.

Member

frandominguez03 commented Aug 3, 2018

Yes, it has the same function and actually shouldn't, so we don't want to remove it.

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Aug 3, 2018

Yes, it has the same function and actually shouldn't, so we don't want to remove it.

Why show 2 times the same? Just because they are different texts, yet have the same function. I don't get it.

wintstar commented Aug 3, 2018

Yes, it has the same function and actually shouldn't, so we don't want to remove it.

Why show 2 times the same? Just because they are different texts, yet have the same function. I don't get it.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Aug 3, 2018

Member

The first one is shown always, the second one is shown after you select the "Extra debuf information" option at the beginning of upgrade proccess. They're not the same.

Member

frandominguez03 commented Aug 3, 2018

The first one is shown always, the second one is shown after you select the "Extra debuf information" option at the beginning of upgrade proccess. They're not the same.

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Aug 4, 2018

Then it would be better to hide the above display in debug mode and display the debug time there.

wintstar commented Aug 4, 2018

Then it would be better to hide the above display in debug mode and display the debug time there.

@Yoshi2889

This comment has been minimized.

Show comment
Hide comment
@Yoshi2889

Yoshi2889 Aug 14, 2018

Contributor

Isn't it possible to split the time into x minutes and y seconds where x and y are different variables? Then you could simply sprintf() (or the javascript equivalent) the variables into the string, problem solved.

EDIT: Looking at the code we have diffMinutes and diffSeconds, even diffHours (seems a bit extreme though, but okay). We could use two strings, 'completed_in_with_hours' and 'completed_in' or something, with 'Completed in %d hours, %d minutes and %d seconds' and 'Completed in %d minutes and %d seconds' respectively.

Contributor

Yoshi2889 commented Aug 14, 2018

Isn't it possible to split the time into x minutes and y seconds where x and y are different variables? Then you could simply sprintf() (or the javascript equivalent) the variables into the string, problem solved.

EDIT: Looking at the code we have diffMinutes and diffSeconds, even diffHours (seems a bit extreme though, but okay). We could use two strings, 'completed_in_with_hours' and 'completed_in' or something, with 'Completed in %d hours, %d minutes and %d seconds' and 'Completed in %d minutes and %d seconds' respectively.

@jdarwood007 jdarwood007 self-assigned this Aug 14, 2018

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Aug 14, 2018

Member

I will look into this. I added this to the upgrader.

Member

jdarwood007 commented Aug 14, 2018

I will look into this. I added this to the upgrader.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Aug 16, 2018

Member

Ok, I think this should be changed as follows.

There is 2 places this occurs in, one is in debug mode after the db upgrade is done. The second one is at the very end.

The debug mode one, we can let it be a little more simple, as its mainly used for development purposes to understand how long the database upgrade part took (actually time from start till db upgrader finished actually).

That language string becomes:

$txt['upgrade_success_time_db'] = 'Successful! Completed in %1$d hours, %1$d minutes and %3$d seconds.';

That code becomes:

			if ($is_debug)
			{
				$active = time() - $upcontext['started'];
				$hours = floor($active / 3600);
				$minutes = intval(($active / 60) % 60);
				$seconds = intval($active % 60);

				echo '', sprintf($txt['upgrade_success_time_db'], $hours, $minutes, $seconds), '<br>';
			}
			else
				echo '', $txt['upgrade_success'], '<br>';

The second one needs to be a bit more friendly since it is intended for general upgrades to know how long the entire process took.

So those language strings

$txt['upgrade_completed_time_hms'] = 'Completed in %1$d hours, %2$s minutes and %3$s seconds';
$txt['upgrade_completed_time_ms'] = 'Completed in %2$s minutes and %3$s seconds';
$txt['upgrade_completed_time_s'] = 'Completed in %3$s seconds';

And that code becomes:

			if ($upcontext['current_debug_item_num'] == $upcontext['debug_items'])
			{
				$active = time() - $upcontext['started'];
				$hours = floor($active / 3600);
				$minutes = intval(($active / 60) % 60);
				$seconds = intval($active % 60);

				$totalTime = '';
				if ($hours > 0)
					$totalTime .= $hours . ' hour' . ($hours > 1 ? 's' : '') . ' ';
				if ($minutes > 0)
					$totalTime .= $minutes . ' minute' . ($minutes > 1 ? 's' : '') . ' ';
				if ($seconds > 0)
					$totalTime .= $seconds . ' second' . ($seconds > 1 ? 's' : '') . ' ';
			}

			echo '
					<p id="upgradeCompleted">';

			if (!empty($totalTime) && $hours > 0)
				echo '', sprintf($txt['upgrade_completed_time_hms'], $hours, $minutes, $seconds), '';
			elseif (!empty($totalTime) && $minutes > 0)
				echo '', sprintf($txt['upgrade_completed_time_ms'], $minutes, $seconds), '';
			elseif (!empty($totalTime) && $seconds > 0)
				echo '', sprintf($txt['upgrade_completed_time_s'], $seconds), '';

I think this gets us what we need. There isn't a elegant way without adding a 4th string to do hours without minutes but have seconds, or just minutes. I think it will be best to accept it saying 0 seconds or 0 minutes in those cases. Otherwise we will have to add more language strings to make things "correct".

We could also format the time as HH:MM:SS and say it like "Upgrade completed in 02:32:16". Looks a bit weird but simple language handling then.

Edit,

I flipped around the 2 codes, but the concept still applies. The db one is the one we actually always show, the end upgrader one is the one only for debug mode.

Member

jdarwood007 commented Aug 16, 2018

Ok, I think this should be changed as follows.

There is 2 places this occurs in, one is in debug mode after the db upgrade is done. The second one is at the very end.

The debug mode one, we can let it be a little more simple, as its mainly used for development purposes to understand how long the database upgrade part took (actually time from start till db upgrader finished actually).

That language string becomes:

$txt['upgrade_success_time_db'] = 'Successful! Completed in %1$d hours, %1$d minutes and %3$d seconds.';

That code becomes:

			if ($is_debug)
			{
				$active = time() - $upcontext['started'];
				$hours = floor($active / 3600);
				$minutes = intval(($active / 60) % 60);
				$seconds = intval($active % 60);

				echo '', sprintf($txt['upgrade_success_time_db'], $hours, $minutes, $seconds), '<br>';
			}
			else
				echo '', $txt['upgrade_success'], '<br>';

The second one needs to be a bit more friendly since it is intended for general upgrades to know how long the entire process took.

So those language strings

$txt['upgrade_completed_time_hms'] = 'Completed in %1$d hours, %2$s minutes and %3$s seconds';
$txt['upgrade_completed_time_ms'] = 'Completed in %2$s minutes and %3$s seconds';
$txt['upgrade_completed_time_s'] = 'Completed in %3$s seconds';

And that code becomes:

			if ($upcontext['current_debug_item_num'] == $upcontext['debug_items'])
			{
				$active = time() - $upcontext['started'];
				$hours = floor($active / 3600);
				$minutes = intval(($active / 60) % 60);
				$seconds = intval($active % 60);

				$totalTime = '';
				if ($hours > 0)
					$totalTime .= $hours . ' hour' . ($hours > 1 ? 's' : '') . ' ';
				if ($minutes > 0)
					$totalTime .= $minutes . ' minute' . ($minutes > 1 ? 's' : '') . ' ';
				if ($seconds > 0)
					$totalTime .= $seconds . ' second' . ($seconds > 1 ? 's' : '') . ' ';
			}

			echo '
					<p id="upgradeCompleted">';

			if (!empty($totalTime) && $hours > 0)
				echo '', sprintf($txt['upgrade_completed_time_hms'], $hours, $minutes, $seconds), '';
			elseif (!empty($totalTime) && $minutes > 0)
				echo '', sprintf($txt['upgrade_completed_time_ms'], $minutes, $seconds), '';
			elseif (!empty($totalTime) && $seconds > 0)
				echo '', sprintf($txt['upgrade_completed_time_s'], $seconds), '';

I think this gets us what we need. There isn't a elegant way without adding a 4th string to do hours without minutes but have seconds, or just minutes. I think it will be best to accept it saying 0 seconds or 0 minutes in those cases. Otherwise we will have to add more language strings to make things "correct".

We could also format the time as HH:MM:SS and say it like "Upgrade completed in 02:32:16". Looks a bit weird but simple language handling then.

Edit,

I flipped around the 2 codes, but the concept still applies. The db one is the one we actually always show, the end upgrader one is the one only for debug mode.

jdarwood007 added a commit to jdarwood007/SMF2.1 that referenced this issue Aug 16, 2018

i18n upgrader fixes for time
This fixes #4889
Implants it in a simple way, but not over complex because its just a upgrader.

@jdarwood007 jdarwood007 closed this in #4923 Aug 19, 2018

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