public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH installer] fix #4430: add UTC timezone as option to installer
@ 2023-03-15 12:26 Christoph Heiss
  2023-03-15 13:23 ` Thomas Lamprecht
  0 siblings, 1 reply; 3+ messages in thread
From: Christoph Heiss @ 2023-03-15 12:26 UTC (permalink / raw)
  To: pve-devel

The 'Etc/UTC' timezone does not have a definite 2-letter country code
assigned. 'xx' was choosen on the basis that this hopefully will never
be assigned to any real country in the future, but a small collision
check won't hurt either.

This also means it does not have an entry in either the ISO-codes
definition file nor the zoneinfo table, thus needing to define it
manually.

Using just 'UTC' as timezone (name) also matches what PVE/PMG/PBS do in
their UI.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Some bike-shedding: If there is a better suggestion on what country code
to choose, I'll happily change it.

Also, although the correct timezone name is 'Etc/UTC', I chose 'UTC' as
"country name" to display in the installer to make it easier to find for
users, as they probably will just type 'UTC' in there at first, rather
than 'Etc'.

 country.pl  | 4 ++++
 proxinstall | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/country.pl b/country.pl
index b1a2d62..ffa47d2 100755
--- a/country.pl
+++ b/country.pl
@@ -72,3 +72,7 @@ foreach my $cc (sort keys %$country) {
     my $mir = $mirrors->{$cc} || '';
     print "$cc:$country->{$cc}:$map:$mir:\n";
 }
+
+die "UTC fake country code collision: $country->{xx}\n"
+    if defined($country->{xx});
+print "xx:UTC:::\n";
diff --git a/proxinstall b/proxinstall
index 79abc34..9000178 100755
--- a/proxinstall
+++ b/proxinstall
@@ -690,6 +690,10 @@ sub read_cmap {
     }
     close ($TMP);

+    $cczones->{xx}->{UTC} = 1;
+    $country->{xx}->{zone} = 'UTC';
+    $zones->{UTC} = 1;
+
     return {
 	zones => $zones,
 	cczones => $cczones,
--
2.39.2





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH installer] fix #4430: add UTC timezone as option to installer
  2023-03-15 12:26 [pve-devel] [PATCH installer] fix #4430: add UTC timezone as option to installer Christoph Heiss
@ 2023-03-15 13:23 ` Thomas Lamprecht
  2023-03-16  9:17   ` Christoph Heiss
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Lamprecht @ 2023-03-15 13:23 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christoph Heiss

Am 15/03/2023 um 13:26 schrieb Christoph Heiss:
> The 'Etc/UTC' timezone does not have a definite 2-letter country code
> assigned. 'xx' was choosen on the basis that this hopefully will never
> be assigned to any real country in the future, but a small collision
> check won't hurt either.
> 
> This also means it does not have an entry in either the ISO-codes
> definition file nor the zoneinfo table, thus needing to define it
> manually.
> 
> Using just 'UTC' as timezone (name) also matches what PVE/PMG/PBS do in
> their UI.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Some bike-shedding: If there is a better suggestion on what country code
> to choose, I'll happily change it.

Something like xx is fine in general, it's the simplest way to integrate this,
so I'd base the exact used letters purely on sorting, 

> 
> Also, although the correct timezone name is 'Etc/UTC', I chose 'UTC' as
> "country name" to display in the installer to make it easier to find for
> users, as they probably will just type 'UTC' in there at first, rather
> than 'Etc'.
> 

That's fine too.


What I don't like as much that one has to set the country to the timezone,
which is confusing UX and will be subtle to most.

Better ways might be:

- add an explicit "Use UTC" checkbox that grey's out the country/timezone
  selection then. Disadvantage here would be taking up extra space and expanding
  the user input amount, which goes a bit against our "as simple as possible"
  approach for the installer

- Add UTC always to the time-zone selection, independent of what country is
  selected. This keeps the selection where it belongs, and allows to quickly
  change to UTC without any typing/searching required (at least for most countries)

From a gut feeling I'd go for the second option, but didn't checked out how the
implementation would then look like.




^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH installer] fix #4430: add UTC timezone as option to installer
  2023-03-15 13:23 ` Thomas Lamprecht
@ 2023-03-16  9:17   ` Christoph Heiss
  0 siblings, 0 replies; 3+ messages in thread
From: Christoph Heiss @ 2023-03-16  9:17 UTC (permalink / raw)
  To: Thomas Lamprecht; +Cc: Proxmox VE development discussion

Thanks for the review!

On Wed, Mar 15, 2023 at 02:23:09PM +0100, Thomas Lamprecht wrote:
> [..]
>
> What I don't like as much that one has to set the country to the timezone,
> which is confusing UX and will be subtle to most.
>
> Better ways might be:
>
> - add an explicit "Use UTC" checkbox that grey's out the country/timezone
>   selection then. Disadvantage here would be taking up extra space and expanding
>   the user input amount, which goes a bit against our "as simple as possible"
>   approach for the installer
>
> - Add UTC always to the time-zone selection, independent of what country is
>   selected. This keeps the selection where it belongs, and allows to quickly
>   change to UTC without any typing/searching required (at least for most countries)
>
> From a gut feeling I'd go for the second option, but didn't checked out how the
> implementation would then look like.
From looking at the code while implementing this, the second option
shouldn't be that much of a hassle to implement I'd say (and probably
less than the first).

Sounds like the better option to me too. The first option could also
strike some users that this option is permament and cannot be changed
after installation, due to the grey'ing out and such, I guess.

I send a v2 soon with the second approach implemented, let's see how the
look & feel of that is.




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-03-16  9:17 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-15 12:26 [pve-devel] [PATCH installer] fix #4430: add UTC timezone as option to installer Christoph Heiss
2023-03-15 13:23 ` Thomas Lamprecht
2023-03-16  9:17   ` Christoph Heiss

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal