Make "scratchpad show" return correct info #3230

hwangcc23 · 2018-04-06T16:05:41Z

Fix the issue #3227(#3227).

Airblader · 2018-04-06T16:08:45Z

I'm sorry, I don't understand the fix. We want to return whether we have successfully shown a window or not. That means we need to use the information coming from scratchpad_show.

hwangcc23 · 2018-04-06T16:39:38Z

Sorry. Misunderstood the problem.
Up.

Airblader

Thanks for the update! There's a couple of changes and I also think we can & should add tests for all these different cases :-)

Airblader · 2018-04-08T13:54:49Z

include/scratchpad.h

@@ -28,8 +28,10 @@ void scratchpad_move(Con *con);
 * scratchpad window, this serves as a shortcut to hide it again (so the user
 * can press the same key to quickly look something up).
 *
+ * Return true if a window is successfully showning.


There's a typo, but let's make change it to

Returns true either if con was in the scratchpad and is now shown or if con was moved back into the scratchpad due to toggling.

Airblader · 2018-04-08T13:55:38Z

src/commands.c

    } else {
        TAILQ_FOREACH(current, &owindows, owindows) {
            DLOG("matching: %p / %s\n", current->con, current->con->name);
-            scratchpad_show(current->con);
+            result = scratchpad_show(current->con);


We should report a failure if there was a failure for any matched window (that's how we try to do it everywhere), so this should be result &= ….

Airblader · 2018-04-08T13:56:13Z

src/scratchpad.c

@@ -83,8 +83,10 @@ void scratchpad_move(Con *con) {
 * scratchpad window, this serves as a shortcut to hide it again (so the user
 * can press the same key to quickly look something up).
 *
+ * Return true if a window is successfully showning.


Same as above.

Airblader · 2018-04-08T13:56:55Z

src/scratchpad.c

@@ -97,7 +99,7 @@ void scratchpad_show(Con *con) {
        floating->scratchpad_state != SCRATCHPAD_NONE) {
        DLOG("Focused window is a scratchpad window, hiding it.\n");
        scratchpad_move(focused);
-        return;
+        return false;


This should return true. I think the true/false semantics should be that it's true whenever we successfully did something with con.

Airblader · 2018-04-08T13:58:52Z

src/scratchpad.c

@@ -165,7 +167,7 @@ void scratchpad_show(Con *con) {
        if (current == active) {
            DLOG("Window is a scratchpad window, hiding it.\n");
            scratchpad_move(con);
-            return;
+            return false;


Should be true.

hwangcc23 · 2018-04-09T16:02:07Z

I am having trouble on implementing the test code for IPC.
Is there anyone having the suggestion or hint?
Thanks.

Here is my test code. I cannot get the returned value of 'scratchpad show' correctly in the test code.

sub scratchpad_show_subtest {
    my ($expect) = @_;
    my @events = events_for(
        sub { cmd 'scratchpad show' },
        'floating_nodes');
    my @temp = grep { $_->{scratchpad_state} eq 'change' } @events;
    is(scalar @temp, $expect, 'Success');
}
fresh_workspace;
open_window;
cmd 'move scratchpad';
subtest 'scratchpad show', \&scratchpad_show_subtest, 1;

orestisfl · 2018-04-17T06:29:16Z

@hwangcc23

fresh_workspace;
open_window;
cmd 'move scratchpad';
my $result = cmd 'scratchpad show';
is($result->[0]->{success}, 1, 'call to scratchpad succeeded');

kill_all_windows;
$result = cmd 'scratchpad show';
is($result->[0]->{success}, 0, 'call to scratchpad failed');

hwangcc23 · 2018-04-17T15:39:32Z

src/commands.c

    } else {
        TAILQ_FOREACH(current, &owindows, owindows) {
            DLOG("matching: %p / %s\n", current->con, current->con->name);
-            scratchpad_show(current->con);
+            result |= scratchpad_show(current->con);


We should report a success if there is any matched window shown or hidden (due to toggled).
This is for passing the test with the criteria(The test case 2: Verify that 'scratchpad show' with the criteria returns correct info).

hwangcc23 · 2018-04-17T15:40:11Z

Up

orestisfl · 2018-04-17T21:49:51Z

testcases/t/298-scratchpad-show.t

+# vim:ts=4:sw=4:expandtab
+#
+# Please read the following documents before working on tests:
+# �~@� https://build.i3wm.org/docs/testsuite.html


Why not extend 185-scratchpad.t? Your â~@¢ character seems to be wrong here too.

orestisfl · 2018-04-17T21:53:04Z

testcases/t/298-scratchpad-show.t

+# 2: Verify that 'scratchpad show' with the criteria returns correct info.
+################################################################################
+my $ws = fresh_workspace;
+cmd 'workspace $ws';


What you do here: open a new workspace with a random name, then immediately switch to a workspace named $ws (you used single quotes). You don't need a variable or the cmd so I think you can remove both.

orestisfl · 2018-04-17T21:54:31Z

testcases/t/298-scratchpad-show.t

+################################################################################
+kill_all_windows;
+
+cmd "workspace foo-1";


nitpick: just use fresh_workspace

orestisfl · 2018-04-17T21:54:40Z

testcases/t/298-scratchpad-show.t

+open_window;
+cmd 'move scratchpad';
+
+cmd "workspace foo-2";


hwangcc23 · 2018-04-18T14:14:25Z

@orestisf1993 Thanks for the review.

Up.

orestisfl · 2018-04-19T16:23:20Z

testcases/t/185-scratchpad.t

+################################################################################
+# 15: Verify that 'scratchpad show' returns correct info.
+################################################################################
+kill_all_windows;


nitpicky but can you add a newline after the comment blocks, like the rest of the file does?

OK. Will send the PR again.

orestisfl · 2018-04-19T16:40:07Z

src/scratchpad.c

@@ -83,8 +83,11 @@ void scratchpad_move(Con *con) {
 * scratchpad window, this serves as a shortcut to hide it again (so the user
 * can press the same key to quickly look something up).
 *
+ * Returns true either if con was in the scratchpad and is now shown or if con


This is slightly inaccurate because true is also returned if there is an unfocused scratchpad on the current workspace. We don't really need to document the return value since it's a bit obvious.

Fix the issue i3#3227(i3#3227). 1).Make cmd_scratchpad_show() use the information coming from scratchpad_show(). 2).Add a test case 298-scratchpad-show.t.

hwangcc23 · 2018-04-20T14:42:55Z

Requested changes are all done.

orestisfl · 2018-04-20T14:51:50Z

Thanks. I'd merge it but I'll let @Airblader have a final look.

Airblader · 2018-04-21T15:58:02Z

Thanks!

Airblader added the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label Apr 6, 2018

hwangcc23 force-pushed the fix-3227 branch from 2e349ea to 3b9509f Compare April 6, 2018 16:35

Airblader requested changes Apr 8, 2018

View reviewed changes

hwangcc23 force-pushed the fix-3227 branch from 3b9509f to b5166cf Compare April 9, 2018 15:31

hwangcc23 force-pushed the fix-3227 branch from b5166cf to 74a5387 Compare April 17, 2018 15:30

hwangcc23 commented Apr 17, 2018

View reviewed changes

orestisfl requested changes Apr 17, 2018

View reviewed changes

hwangcc23 force-pushed the fix-3227 branch from 74a5387 to 0ad1e14 Compare April 18, 2018 14:01

orestisfl approved these changes Apr 19, 2018

View reviewed changes

Make "scratchpad show" return correct info

4869bec

Fix the issue i3#3227(i3#3227). 1).Make cmd_scratchpad_show() use the information coming from scratchpad_show(). 2).Add a test case 298-scratchpad-show.t.

hwangcc23 force-pushed the fix-3227 branch from 0ad1e14 to 4869bec Compare April 20, 2018 14:31

Airblader approved these changes Apr 21, 2018

View reviewed changes

Airblader merged commit 5a0f02b into i3:next Apr 21, 2018

hwangcc23 deleted the fix-3227 branch April 23, 2018 04:01

orestisfl removed the waiting Waiting for feedback/changes from the author of the issue/PR. Make sure to notify us with a comment. label Jan 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make "scratchpad show" return correct info #3230

Make "scratchpad show" return correct info #3230

hwangcc23 commented Apr 6, 2018

Airblader commented Apr 6, 2018

hwangcc23 commented Apr 6, 2018

Airblader left a comment

Airblader Apr 8, 2018

Airblader Apr 8, 2018

Airblader Apr 8, 2018

Airblader Apr 8, 2018

Airblader Apr 8, 2018

hwangcc23 commented Apr 9, 2018

orestisfl commented Apr 17, 2018

hwangcc23 Apr 17, 2018

hwangcc23 commented Apr 17, 2018

orestisfl Apr 17, 2018

orestisfl Apr 17, 2018

orestisfl Apr 17, 2018

orestisfl Apr 17, 2018

hwangcc23 commented Apr 18, 2018

orestisfl Apr 19, 2018

hwangcc23 Apr 20, 2018

orestisfl Apr 19, 2018

hwangcc23 commented Apr 20, 2018

orestisfl commented Apr 20, 2018

Airblader commented Apr 21, 2018

Make "scratchpad show" return correct info #3230

Make "scratchpad show" return correct info #3230

Conversation

hwangcc23 commented Apr 6, 2018

Airblader commented Apr 6, 2018

hwangcc23 commented Apr 6, 2018

Airblader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwangcc23 commented Apr 9, 2018

orestisfl commented Apr 17, 2018

Choose a reason for hiding this comment

hwangcc23 commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwangcc23 commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwangcc23 commented Apr 20, 2018

orestisfl commented Apr 20, 2018

Airblader commented Apr 21, 2018