Skip to content

feat(kill-process): allow killing multiple processes#9185

Merged
pernielsentikaer merged 16 commits intoraycast:mainfrom
erics118:kill-process/multiple
Nov 12, 2023
Merged

feat(kill-process): allow killing multiple processes#9185
pernielsentikaer merged 16 commits intoraycast:mainfrom
erics118:kill-process/multiple

Conversation

@erics118
Copy link
Contributor

@erics118 erics118 commented Nov 9, 2023

Description

pretty simple. it just adds a preference to not close the main window after killing a process

Screencast

CleanShot.2023-11-09.at.14.29.33.mp4

Checklist

@erics118 erics118 changed the title feat: allow killing multiple processes feat(kill-process): allow killing multiple processes Nov 9, 2023
@raycastbot raycastbot added extension fix / improvement Label for PRs with extension's fix improvements extension: kill-process Issues related to the kill-process extension labels Nov 9, 2023
@raycastbot
Copy link
Collaborator

raycastbot commented Nov 9, 2023

Thank you for your contribution! 🎉

🔔 @rolandleth @crazyones110 @zhenpewu @Saafo you might want to have a look.

clearSearchBar replaced with popToRoot after closeMainWindow
@erics118 erics118 force-pushed the kill-process/multiple branch from ccba3a2 to 0b2a4c4 Compare November 9, 2023 23:49
clearSearchBar({ forceScrollToTop: true });
showHUD(`✅ Killed ${process.processName === "-" ? `process ${process.id}` : process.processName}`);
if (!multipleKills) {
closeMainWindow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding close main window, personally, I think it is not needed, but other reviewers might have different thoughts.

Copy link
Contributor

@rolandleth rolandleth Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's needed either, for me it already closes it, so I think it just respects Raycast's settings.

But without this change, there's nothing the PR would add, so how would this preference work? And since for me it already closes it, this bit of code wouldn't do anything for me, so for others who have the same settings as me, this preference would seem broken.

Copy link
Contributor

@zhenpewu zhenpewu Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we all agree it is not necessary to close window, I don't think multiple kills pref is needed, as there is no any diff between single and multiple kills.

the main change for this PR is to migrate from showHUD to showToast, as showHUD will close window directly, which would be annoying while kill multiple processes.

Copy link
Contributor

@rolandleth rolandleth Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know that showHUD is actually what's closing the window. In that case, allowMultipleKills doesn't make sense anymore, no? With showToast it will always leave the window open and you can do multiple kills.

What would make sense would be a "Close window after kill" preference, default to true, because most of the time/people need to kill one process and that's it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I've changed the preference to closeWindowAfterKill, with a default to true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pernielsentikaer Yup, that's what we're discussing here 😄

@erics118 erics118 force-pushed the kill-process/multiple branch from 50376a0 to d27d53d Compare November 10, 2023 00:59
@pernielsentikaer
Copy link
Collaborator

That's some good feedback, so we're switching to a toast that doesn't close the window - we could have a preference for letting the user chose to close the window (which would do as today) and let them toggle it off to leave open?

@pernielsentikaer pernielsentikaer self-assigned this Nov 10, 2023
this should not change the behavior. the original code essentially gave
a default of false in the extension code, but now it's set in the actual
preferences
the search text is always defined as a string, so there's no need for an
`undefined` state
@erics118
Copy link
Contributor Author

I believe I've implemented the requested changes

Copy link
Collaborator

@pernielsentikaer pernielsentikaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋

Looks good to me, approved 🔥

@pernielsentikaer pernielsentikaer merged commit 5a430f7 into raycast:main Nov 12, 2023
@raycastbot
Copy link
Collaborator

Published to the Raycast Store:
https://raycast.com/rolandleth/kill-process

@raycastbot
Copy link
Collaborator

🎉 🎉 🎉

We've rewarded your Raycast account with some credits. You will soon be able to exchange them for some swag.

@erics118 erics118 deleted the kill-process/multiple branch November 12, 2023 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extension fix / improvement Label for PRs with extension's fix improvements extension: kill-process Issues related to the kill-process extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants