Platform handling improvements and WSL bugfix. #1059
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zulipbot
added
size: L
neiljp
reviewed
Jul 1, 2021
There was a problem hiding this comment.
@zee-bit Some initial thoughts: (possibly some are in the issue?)
- I think a good long-term goal would be to have all the platform-specific code in one module, and it's likely easier for that to be where the detection happens too; on that basis and given the detection would then be a detail of the module, I'm not entirely convinced it fits in
config/. - While it's not always possible, it would be good to see this first commit not just add unused code but migrate to using it - ie. be a refactor of the detection into the new file, with the old platform variables being set according to
PLATFORM- the second commit would then become a migration from those to usePLATFORM(so be slightly smaller). If we go the approach of one module for minimal platform-specific functions, that could then be a good follow-up. - We don't currently test the platform detection, which I think we could do with marks/skips; that's maybe not explicitly part of TRACKING: Improve platform handling #791 but we should test it more explicitly if we can.
- You've reused the Literal definition in multiple places which would normally make me want to define it instead, along with the multi-part definition I suggested on the stream (like we do with Flags), to leave "unsupported" separate.
- Another follow-up might be to add an extra helper to the module to check the platform is supported and/or give details of that
neiljp
added
PR awaiting update
zee-bit
force-pushed
the
improve-platform-handling
branch
from
e70c224 to
210a9eb
Compare
zee-bit
force-pushed
the
improve-platform-handling
branch
from
210a9eb to
cb6a7d2
Compare
zulipbot
added
size: XL
This commit creates a new module (platform_code.py) inside zulipterminal/ which contains the code to detect the current platform (OS) that the ZT instance is running on. The platform_code.py module has a PLATFORM variable that holds the platform name which could only be one of "Linux", "MacOS", "WSL" or "unsupported". Currently, we only use this to set our old platform variables in helper.py accordingly.
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.
This commit shifts the notify() function to the platform_code.py module in an effort to move all platform dependent code to a single isolated module so that other modules are unaware of the underlying platform and how they are handled for different platforms. This will also allow us to better handle unsupported platforms. Tests amended. New test file created, and added to list of files to be linted by mypy.
This commit moves the platform dependent selection_key condition to the platform_code.py module. The selection key is now stored inside a global variable MOUSE_SELECTION_KEY that can be imported in other modules whenever needed.
neiljp
force-pushed
the
improve-platform-handling
branch
from
cb6a7d2 to
464faf6
Compare
|
Hello @zee-bit, it seems like you have referenced #791 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged. Please run An example of a correctly-formatted commit: Thank you for your contributions to Zulip! |
5 tasks
|
@zee-bit Thanks for this refactor/bugfix 👍 I'm going to merge this now after a few minor adjustments 🎉 While this migrates almost all I've left #791 from being closed by this, since while this does resolve the multiple-platform issue, it also mentions other issues, which I've expanded on in a task list there after the discussion in the stream. |
neiljp
added
area: refactoring
bug
|
Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves our platform handling by defining an isolated module
config/platform_detection.pythat contains the platform detection code and uses a single variablePLATFORMto hold the name of the platform that the current ZT instance is loaded on. This fixes a bug in WSL where multiple platforms were detected. Related discussion in: #zulip-terminal>Platform handling (#T791) and WSL issuesFixes #791.