Show detected platform on startup and in About popup #1221

plugyawn · 2022-05-06T14:29:06Z

What does this PR do?
Shows the detected platform on startup and in the About pop-up.

Tries to fix #1216.

Tested?

Manually
Existing tests (adapted, if necessary)
New tests added (for any new behavior)
Passed linting & tests (each commit)

Commit flow

Add platform information on startup
Add platform information to About menu

Notes & Questions

Interactions

Visual changes

zulipbot · 2022-05-06T14:29:09Z

Hello @plugyawn, it seems like you have referenced #1216 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #1216..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

neiljp

@plugyawn Thanks for adding this, when ready it should be helpful for debugging in future 👍

Generally it may be useful to use something along the lines of Detected Platform instead of the 'Running' and 'Platform' you currently have, both for consistency, and since this is what ZT determines is the platform - we can't know for sure.

This doesn't currently address the idea of showing extra text if the platform is unsupported, from the original issue; you didn't mention skipping it for now in the PR description, so I assume you missed that.

I think I mentioned this in another PR review, but I'd recommend reading your PRs in GitHub and maybe also something like tig or another local viewer, to ensure that the commit text and contents looks like what you'd expect.

zulipterminal/core.py

neiljp · 2022-05-06T21:39:49Z

zulipterminal/ui_tools/views.py

        )
        contents = [
-            ("Application", [("Zulip Terminal", zt_version)]),
+            ("Application", [("Zulip Terminal", zt_version), ("Platform", PLATFORM)]),


This looks OK, though I had originally envisaged it being in a separate section below the others, rather than in the Application section.

I assumed the version and the platform would go together? I'll put it in a separate section, that does sound better.

plugyawn · 2022-05-08T10:03:04Z

@plugyawn Thanks for adding this, when ready it should be helpful for debugging in future 👍

Generally it may be useful to use something along the lines of Detected Platform instead of the 'Running' and 'Platform' you currently have, both for consistency, and since this is what ZT determines is the platform - we can't know for sure.

This doesn't currently address the idea of showing extra text if the platform is unsupported, from the original issue; you didn't mention skipping it for now in the PR description, so I assume you missed that.

I think I mentioned this in another PR review, but I'd recommend reading your PRs in GitHub and maybe also something like tig or another local viewer, to ensure that the commit text and contents looks like what you'd expect.

I missed the part about checking if the platform is unsupported, sorry -- I'm adding it right now.

Shows PLATFORM from platform_code.py on the About Pop-Up.

Adds tests for checking what platform is detected.

neiljp

@plugyawn You didn't mark this for review, but here's some feedback since you mentioned updating it in the stream. Please do toggle the labels if you're seeking review.

neiljp · 2022-05-08T18:01:01Z

zulipterminal/cli/run.py

+        if PLATFORM:
+            print("Detected Platform: " + PLATFORM)
+        else:
+            print("Running on undetected platform.")


Is this intended to be for unsupported platforms, or something else?

Please make sure you're aware what values this can take :)

You're not testing this result, in any case.

Is this intended to be for unsupported platforms, or something else?

Please make sure you're aware what values this can take :)

You're not testing this result, in any case.

I'll look into that; I'm still not exactly sure how to check for unsupported platforms, though.

neiljp · 2022-05-10T01:43:12Z

zulipterminal/ui_tools/views.py

+                "Application",
+                [
+                    ("Zulip Terminal", zt_version),
+                    ("Platform detected", PLATFORM if PLATFORM else "Invalid platform"),


This condition shouldn't arise - again, check the values of PLATFORM.

neiljp · 2022-05-10T01:44:23Z

tests/cli/test_run.py

    captured = capsys.readouterr()
    lines = captured.out.strip().split("\n")
    expected_lines = [
+        f"Detected Platform: {PLATFORM}",


Is there a reason you used different code here?

None, I'll change it to the same code as the others.

neiljp · 2022-05-10T01:46:04Z

tests/cli/test_run.py


    lines = captured.out.strip().split("\n")
    expected_lines = [
+        "Detected Platform: " + PLATFORM,


Note that this will be the platform that the tests are running on. We may want to mock this instead to ensure we get some known output (and avoid the running vs testing confusion for readers).

It's good to keep tests in the same commit as the code that changes, as it's a unit of change, so here combined with one of the other commits.

We may want a specific test to handle how this output should vary on different platforms, including unsupported.

zulipbot · 2023-02-24T21:17:35Z

Heads up @plugyawn, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2023-11-06T23:52:21Z

@plugyawn Thanks for your work on this 👍 I recently merged #1435, which includes the About popup element, so I'm closing this now.

zulipbot added the size: XS [Automatic label added by zulipbot] label May 6, 2022

plugyawn requested a review from neiljp May 6, 2022 14:29

neiljp reviewed May 6, 2022

View reviewed changes

neiljp added area: UI General user interface update PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels May 6, 2022

run: Add platform information on startup.

f1db4d4

plugyawn force-pushed the T1216_platform branch from a893897 to 117b62d Compare May 8, 2022 11:20

zulipbot added size: S [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels May 8, 2022

plugyawn force-pushed the T1216_platform branch 5 times, most recently from ccede63 to 16f92fe Compare May 9, 2022 02:31

zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels May 9, 2022

plugyawn added 2 commits May 9, 2022 08:08

views: Add platform information to the About Pop-Up.

e405fbd

Shows PLATFORM from platform_code.py on the About Pop-Up.

test: Add tests for detected platform.

9857ec0

Adds tests for checking what platform is detected.

plugyawn force-pushed the T1216_platform branch from 16f92fe to 9857ec0 Compare May 9, 2022 02:39

zulipbot added size: S [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels May 9, 2022

neiljp reviewed May 10, 2022

View reviewed changes

plugyawn closed this Jun 17, 2022

plugyawn reopened this Jun 17, 2022

zulipbot added the has conflicts label Feb 24, 2023

neiljp added the PR replaced by another PR label Nov 6, 2023

neiljp closed this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Show detected platform on startup and in About popup #1221

Show detected platform on startup and in About popup #1221

Uh oh!

plugyawn commented May 6, 2022 •

edited

Loading

Uh oh!

zulipbot commented May 6, 2022

Uh oh!

neiljp left a comment

Uh oh!

Uh oh!

neiljp May 6, 2022

Uh oh!

plugyawn May 8, 2022

Uh oh!

plugyawn commented May 8, 2022

Uh oh!

neiljp left a comment

Uh oh!

neiljp May 8, 2022

Uh oh!

plugyawn May 10, 2022

Uh oh!

neiljp May 10, 2022

Uh oh!

neiljp May 10, 2022

Uh oh!

plugyawn May 10, 2022

Uh oh!

neiljp May 10, 2022

Uh oh!

zulipbot commented Feb 24, 2023

Uh oh!

neiljp commented Nov 6, 2023

Uh oh!

Uh oh!

Uh oh!

Show detected platform on startup and in About popup #1221

Show detected platform on startup and in About popup #1221

Uh oh!

Conversation

plugyawn commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zulipbot commented May 6, 2022

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plugyawn commented May 8, 2022

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zulipbot commented Feb 24, 2023

Uh oh!

neiljp commented Nov 6, 2023

Uh oh!

Uh oh!

plugyawn commented May 6, 2022 •

edited

Loading