Enhancement proposal for issue #3162 #3320
Conversation
Added 'scratchpad show [on] output <name>' command.
Sorry about that.
orestisfl
changed the title
|
@Airblader what do you think of this PR on a higher level? If it sounds good to you I might do a first review pass. |
|
Yes, on a high level I think this sounds fine. |
| if (strncmp(name, "__", strlen("__")) == 0) { | ||
| LOG("You cannot show the scratchpad on an i3-internal output (\"%s\").\n", | ||
| name); | ||
| ysuccess(false); |
There was a problem hiding this comment.
Use yerror("You cannot show the scratchpad on an i3-internal output (\"%s\").\n", name); instead of LOG + ysuccess
There was a problem hiding this comment.
Later in the code I use ELOG + ysuccess. Considering there is a difference in the printed message, should I yerror them as well?
There was a problem hiding this comment.
Yeah; if the command failed due to an error, we should use yerror to return this to the client that started the IPC request.
| * focused (or hide_if_visible is true and it is visible on the target output) | ||
| * it will hide the window again. | ||
| */ | ||
| bool scratchpad_show_on_output(Con *con, Output *current_output, Output *output, |
There was a problem hiding this comment.
current_output is not being used
| @@ -210,6 +210,157 @@ bool scratchpad_show(Con *con) { | |||
| return true; | |||
| } | |||
|
|
|||
| /** | |||
| return; | ||
| } | ||
|
|
||
| // TODO: remove duplicate code |
| } | ||
| } | ||
|
|
||
| /* ? is there a reliable way of starting from the most recently focused |
There was a problem hiding this comment.
Why do you need the more recently focused workspace?
There was a problem hiding this comment.
It is not that I necessarily need the more recently focused workspace, I mostly need the current workspace that is on display at the target output. I'd like to check for scratchpad windows on that workspace first, since it'd be the one the user would intuitively like to focus on.
| * focused (or hide_if_visible is true and it is visible on the target output) | ||
| * it will hide the window again. | ||
| */ | ||
| bool scratchpad_show_on_output(Con *con, Output *current_output, Output *output, |
There was a problem hiding this comment.
I feel that we should merge this function with scratchpad_show to reduce duplicate code.
Airblader
added
the
waiting
|
@leoagomes Are you going to continue your work on this? |
|
I can continue to work on this, just not at the moment, as the end of the semester is near and I have to finish multiple projects. When December comes, I'll have time to address the proposed changes. |
|
That's fine, thanks for the reply. Good luck with your studies! |
|
Feel free to reopen & finish this whenever you have the time. Best of luck with your studies! |
orestisfl
added
stale
Added a variant of the
scratchpad showcommand,scratchpad show [--hide-if-visible] [on] output <name>as a possible solution for #3162 .From my comment on the issue:
and
For more of the context, please check the issue.
I tried my best to mirror the styles I saw in source files, but please let me know if anything should be done differently.