Skip to content

TRACKING: Improve platform handling #791

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

Open
3 of 5 tasks
neiljp opened this issue Aug 31, 2020 · 3 comments · Fixed by #1059
Open
3 of 5 tasks

TRACKING: Improve platform handling #791

neiljp opened this issue Aug 31, 2020 · 3 comments · Fixed by #1059
Labels
area: infrastructure Project infrastructure TRACKING

Comments

@neiljp
Copy link
Collaborator

neiljp commented Aug 31, 2020

Currently we use platform to determine which platform we're running on, currently primarily for notifications.

There are currently some limitations in this:

  • while not expected, we don't ensure that these values are mutually exclusive (only one is set)
  • we explicitly only check for (and test on) these platforms

The former means - admittedly in likely rare situations - that we may load multiple/incorrect notifications. Multiple platforms should be an error that either the application or user disambiguates.

The latter means that other platforms, such as likely BSD or more broadly POSIX-like systems may not run properly, or at minimum will eg just transparently not show notifications as it stands.

We currently have the platform detection and notification code in helper.py; I'd suggest we extract this code and have platform-dependent code be isolated in a separate module, with other code not being otherwise aware of any underlying platform, just whether things are working/available or not.

edit
Expected functionality to complete this:

@zee-bit
Copy link
Member

zee-bit commented Dec 24, 2020

@neiljp for the first part of the limitation, we know platform.system() will always be one of 'Linux', 'Darwin', 'Java' or 'Windows', so instead of pre-checking for the correct platform values(which may not be mutually exclusive), we can check for the system on the go inside notify function. Something like this:

if platform.system() is 'Linux':
    notify Linux
elif platform.system() is 'Darwin':
    notify MacOS

This should ensure that notification for the correct platform is always loaded.

For the second part I am guessing the value of platform.system() will be empty, if the platform is BSD or other POSIX-based. So, I am not sure how to handle this limitation. But we can surely discuss this further.

I am hoping to work on this issue, once the above points are properly discussed.

@neiljp
Copy link
Collaborator Author

neiljp commented Jan 10, 2021

The platform should not vary dynamically (ie. while running), so we only need to check once and then look up later - we don't need to keep checking platform.system(). We can do this via the new platform-agnostic wrapper file.

Note that the docs say "such as..." for those values so there may be other values - and this may not always be the best way of identifying the platforms, in the way we do it differently for WSL right now. In any case the point is to hide and centralize this in the new wrapper file so we just request an action from it, rather than looking-up platforms in each location.

Once we have platforms encoded, we can also provide string representations and feature support feedback. For example, what is the current platform identified as? Is the current platform fully supported? What is/is-not supported? This could go into the About popup.

zee-bit added a commit to zee-bit/zulip-terminal that referenced this issue Jun 28, 2021
This commit adapts our codebase to use the new PLATFORM variable instead
of the 3 booleans that stood for each of the supported platforms i.e. WSL,
MACOS and LINUX. This ensures that only one platform (OS) is set for each
instance.

We modify the platform-dependent codes in various modules to use this new
variable, which prevents bugs that arised earlier due to multiple OS'es
being detected in WSL. Discussion about the bug and how it affected some
features in WSL: #zulip-terminal>Platform handling (#T791) and WSL issues.

Tests amended.
Fixes zulip#791.
zee-bit added a commit to zee-bit/zulip-terminal that referenced this issue Jul 7, 2021
This commit adapts our codebase to use the new PLATFORM variable instead
of the 3 booleans that stood for each of the supported platforms i.e. WSL,
MACOS and LINUX. This ensures that only one platform (OS) is set for each
instance i.e. it is mutually exclusive.

We modify the platform-dependent codes in various modules to use this new
variable, which prevents bugs that arised earlier due to multiple OS'es
being detected in WSL. Discussion about the bug and how it affected some
features in WSL: #zulip-terminal>Platform handling (#T791) and WSL issues.

Tests amended.
Fixes zulip#791.
zee-bit added a commit to zee-bit/zulip-terminal that referenced this issue Jul 20, 2021
This commit adapts our codebase to use the new PLATFORM variable instead
of the 3 booleans that stood for each of the supported platforms i.e. WSL,
MACOS and LINUX. This ensures that only one platform (OS) is set for each
instance i.e. it is mutually exclusive.

We modify the platform-dependent codes in various modules to use this new
variable, which prevents bugs that arised earlier due to multiple OS'es
being detected in WSL. Discussion about the bug and how it affected some
features in WSL: #zulip-terminal>Platform handling (#T791) and WSL issues.

Tests amended.
Fixes zulip#791.
neiljp pushed a commit to zee-bit/zulip-terminal that referenced this issue Jul 25, 2021
This commit adapts the codebase to use the new PLATFORM variable instead
of the 3 booleans that stood for each of the supported platforms i.e. WSL,
MACOS and LINUX. This ensures that only one platform (OS) is set for each
instance i.e. it is mutually exclusive.

We modify the platform-dependent codes in various modules to use this new
variable, which prevents bugs that arose earlier due to multiple OS'es
being detected in WSL. Discussion about the bug and how it affected some
features in WSL: #zulip-terminal>Platform handling (#T791) and WSL issues.

Tests amended.

This is a partial fix towards zulip#791.
neiljp pushed a commit that referenced this issue Jul 25, 2021
This commit adapts the codebase to use the new PLATFORM variable instead
of the 3 booleans that stood for each of the supported platforms i.e. WSL,
MACOS and LINUX. This ensures that only one platform (OS) is set for each
instance i.e. it is mutually exclusive.

We modify the platform-dependent codes in various modules to use this new
variable, which prevents bugs that arose earlier due to multiple OS'es
being detected in WSL. Discussion about the bug and how it affected some
features in WSL: #zulip-terminal>Platform handling (#T791) and WSL issues.

Tests amended.

This is a partial fix towards #791.
@neiljp neiljp reopened this Jul 25, 2021
@neiljp neiljp added good first issue Good for newcomers and removed General labels Apr 11, 2022
@neiljp neiljp changed the title Improve platform handling TRACKING: Improve platform handling Apr 11, 2022
@neiljp neiljp added area: infrastructure Project infrastructure and removed good first issue Good for newcomers further discussion required Discuss this on #zulip-terminal on chat.zulip.org labels Apr 29, 2022
@neiljp
Copy link
Collaborator Author

neiljp commented Apr 29, 2022

I just split the UI part out into #1216 making that more accessible, leaving this remaining as infrastructure only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: infrastructure Project infrastructure TRACKING
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants