-
-
Notifications
You must be signed in to change notification settings - Fork 278
Showing Bot Markers. #1187
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
Showing Bot Markers. #1187
Conversation
@plugyawn I mentioned about the symbol in the stream. Otherwise please use the standard commit text style for zulip-terminal as indicated in the README (and see gitlint), and adjust the commit flow ready for merging. This looks straightforward; I'm not sure if we explicitly need tests for this. In a bot-related way, you might consider how the user popup can be improved for the case of bots, as a follow-up. |
On it! I'll update the commit text and let you know asap. |
@plugyawn I'm mainly waiting for a commit reflow here, which may be simple, but just checked functionality again as it stands as this seems a quick PR. However, it seems that the symbol you're using is styled to be black on a white background somehow, and that is bleeding into the user search box names too. That doesn't seem to be intentional, so I wonder if it's part of the symbol itself somehow? |
a4f1961
to
e0f5236
Compare
@neiljp updated the commit flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plugyawn This is tidier, but as per my inline notes, please keep the tidying (and intentional changes) that go with each set of changes in the associated commit. Reading your changes through in the github interface, as if reviewing your own code, will likely show obvious issues like the pawn vs robot unicode codepoint.
We don't always end up needing the commits to be so small, but keeping logical changes separate makes it much easier to read.
Also note that the formatting being in the wrong commit would be picked up if you ran |
e0f5236
to
c39cbd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plugyawn This looks much tidier :)
I think the first two commits may be squashable together later, as they're very small.
This appears to work fine on one machine, but I tested on my new machine (python3.9, debian) and it doesn't show the symbols, but that may be a local issue.
My remaining concerns are if we have other locations where the status is otherwise used which haven't been updated. This is evident since like I mentioned in the last group chat, many locations directly access this data and so it's rather fragile.
One thing we might do to address this is to define the types of the elements of user_dict, and mypy may then be able to work with that, particularly if we use Literal types for the status.
if user["is_bot"]: | ||
self.user_dict[user["email"]] = { | ||
"full_name": user["full_name"], | ||
"email": email, | ||
"user_id": user["user_id"], | ||
"status": "bot", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bots don't have status - hence the marker - so let's avoid doing the work just above this and treat a bot specially, like with the user running the application, in a block further up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you understand my suggestion? You've not replied or updated for it.
See other feedback in the stream. I resolved my issue with it not working at all on one of my computers. |
I have reviewed the PR and have posted some questions in the stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon running the PR, it worked without any glitches/crashes. It didn't work for Notification Bot though but I think this has been discussed in the stream already. The changes look good to me.
@plugyawn This PR would be good to close out, now you've got feedback too? |
b8f09bd
to
830032e
Compare
830032e
to
e83a43f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plugyawn The system bots appear to work now (eg. Notification bot), though I assume you'll check yourself again since you left a comment :)
As you said in the stream, the styles are still strange, which is likely related to the way styles are picked for each user status - like the green/yellow/white. These are connected somewhere, so we likely want to extend the styles to cover the bot case you added.
zulipterminal/model.py
Outdated
@@ -985,7 +994,7 @@ def get_all_users(self) -> List[Dict[str, Any]]: | |||
"full_name": bot["full_name"], | |||
"email": email, | |||
"user_id": bot["user_id"], | |||
"status": "inactive", | |||
"status": "bot", #FIXME was inactive, does this work? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this? See Mounil's comment on the PR to test for that bot user, eg. Notification bot or Welcome bot.
e83a43f
to
e404149
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plugyawn This looks cleaner now 👍
I've left more feedback inline, but also please try and keep the linting clean, and adapt tests appropriately where necessary, ideally on a per-commit basis - keeping those up to date means that the PR is closer to a mergeable state at any given time. Have you been using black or make fix
on commits to tidy these? That should keep the manual tidying load down.
There was a previous comment you didn't address, but that could be a refactor later - it's just us avoiding calculating presence for bots from memory.
zulipterminal/themes/zt_dark.py
Outdated
@@ -24,6 +24,7 @@ | |||
'user_idle' : (Color.YELLOW, Color.BLACK), | |||
'user_offline' : (Color.WHITE, Color.BLACK), | |||
'user_inactive' : (Color.WHITE, Color.BLACK), | |||
'user_bot' : (Color.WHITE, Color.BLACK), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be in more than one style (and aligned) - I still get the previous bad styling/highlighting behavior in eg. gruvbox_dark.
While arguable, I would also place this commit before the other in order to have good styling available before you use it. The other commit may still "work" (if incompletely/wrongly visually), but in addition it reads strangely, specifically like a bugfix.
bot = [ | ||
properties | ||
for properties in self.user_dict.values() | ||
if properties["status"] == "bot" | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different behavior than what you had before, and what we have now, so it at least warrants mentioning what changed in the commit text. I'd almost suggest this could be a separate commit, since it's a different change - the ordering, not the icon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased anda added a new commit.
e404149
to
8d7eb2c
Compare
26dcb75
to
2fee25d
Compare
2fee25d
to
1e036de
Compare
1953af2
to
a9d41e3
Compare
a9d41e3
to
34ac885
Compare
This will be picked up using earlier commits, to show it differently in the UI. With a different status, they are now explicitly placed at the end of the user list. Tests updated. Original logic by Progyan, simplified using logic suggested in review by neiljp in zulip#1187. Co-authored-by: neiljp (Neil Pilgrim) <[email protected]>
This will be picked up using earlier commits, to show it differently in the UI. With a different status, they are now explicitly placed at the end of the user list. Tests updated. Original logic by Progyan, simplified using logic suggested in review by neiljp in zulip#1187. Co-authored-by: neiljp (Neil Pilgrim) <[email protected]>
This will be picked up using earlier commits, to show it differently in the UI. With a different status, bots are now explicitly placed at the end of the user list. Tests updated. Original logic by Progyan, simplified using logic suggested in review by neiljp in zulip#1187. Co-authored-by: neiljp (Neil Pilgrim) <[email protected]>
This will be picked up using earlier commits, to show it differently in the UI. With a different status, bots are now explicitly placed at the end of the user list. Tests updated. Original logic by Progyan, simplified using logic suggested in review by neiljp in #1187. Co-authored-by: neiljp (Neil Pilgrim) <[email protected]>
What does this PR do?
Adds bot markers to bots.
Partial fix for #1155.
Tested?
Commit flow
Notes & Questions
Visual changes