[Kickoff] Return Kickoff to Favorites page after running a search and closing it
ClosedPublic

Authored by rooty on Fri, Feb 8, 12:06 PM.

Details

Summary

This patch fixes the bug that would make Kickoff retain the contents of the tab that was selected when a search was run from a tab other than
Favorites (but at the same time highlight Favorites in the Tab Bar, when it was in fact, stuck on another tab).

BUG: 404088
FIXED-IN: 5.12.8, 5.15.1

Test Plan

Before

After

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rooty created this revision.Fri, Feb 8, 12:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Feb 8, 12:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rooty requested review of this revision.Fri, Feb 8, 12:06 PM
rooty edited the test plan for this revision. (Show Details)Fri, Feb 8, 12:07 PM
rooty added a reviewer: VDG.
rooty edited the test plan for this revision. (Show Details)
rooty edited the summary of this revision. (Show Details)Fri, Feb 8, 12:09 PM
rooty edited the test plan for this revision. (Show Details)
rooty edited the test plan for this revision. (Show Details)Fri, Feb 8, 12:12 PM
rooty added a reviewer: Plasma.
rooty edited the summary of this revision. (Show Details)Fri, Feb 8, 12:17 PM
rooty retitled this revision from [Kickoff] Return Kickoff to Favorites page after running a search to [Kickoff] Return Kickoff to Favorites page after running a search and closing it .
rooty updated this revision to Diff 51176.Fri, Feb 8, 12:21 PM
rooty retitled this revision from [Kickoff] Return Kickoff to Favorites page after running a search and closing it to [Kickoff] Return Kickoff to Favorites page after running a search and closing it.

Remove comment (it doesn't concern the LTS branches that still use "hint")

filipf added a subscriber: filipf.Fri, Feb 8, 12:33 PM

Fixes the bug for me. I also reproduced it in 5.12 so you should add: FIXED-IN: 5.12.8

rooty edited the test plan for this revision. (Show Details)Fri, Feb 8, 12:36 PM
rooty edited the summary of this revision. (Show Details)Fri, Feb 8, 12:39 PM
hein added a subscriber: hein.Fri, Feb 8, 12:44 PM

This is a good change, but it's more worth finding out why it doesn't work already. Looking at the code, when KickoffItem is triggered it runs the switchToInitial() function in FullRepresentation.qml, which already switches tabs and is also what sets the state back to Normal. So setting the tab in the state shouldn't be necessary. Can you debug why setting currentTab in switchToInitial() apparently doesn't work?

GB_2 edited the summary of this revision. (Show Details)Fri, Feb 8, 12:47 PM
rooty added a comment.Fri, Feb 8, 12:51 PM
In D18848#407838, @hein wrote:

This is a good change, but it's more worth finding out why it doesn't work already. Looking at the code, when KickoffItem is triggered it runs the switchToInitial() function in FullRepresentation.qml, which already switches tabs and is also what sets the state back to Normal. So setting the tab in the state shouldn't be necessary. Can you debug why setting currentTab in switchToInitial() apparently doesn't work?

I think it might be because of the switch from the state "Search" to the state "Normal", because this doesn't happen if the arrow keys are used to navigate to the right of Favorites, only when Search is invoked. There's a "currentTab: searchPage" (line 738) setting in the state "Search" that probably gets stuck. I didn't want to deal with changing "Search" because in my experience, Kickoff's search view can be pretty temperamental. This seemed like the safest course of action.

ngraham accepted this revision.Fri, Feb 8, 2:03 PM
ngraham added a subscriber: ngraham.

Nice fix, seems sensible enough to be explicit. Confirmed the bug and that this fixes it.

This revision is now accepted and ready to land.Fri, Feb 8, 2:03 PM

BTW your FIXED-IN tag says 5.12.x, but this patch was branched off of master. So if and when this lands, if you don't do anything special, it's currently going to master only. To land it on 5.12.8, the easiest thing is to just cherry-pick the commit hash onto Plasma/5.12 and merge forward into 5.15 and master. I can help out with that.

rooty added a comment.EditedFri, Feb 8, 3:20 PM

BTW your FIXED-IN tag says 5.12.x, but this patch was branched off of master. So if and when this lands, if you don't do anything special, it's currently going to master only. To land it on 5.12.8, the easiest thing is to just cherry-pick the commit hash onto Plasma/5.12 and merge forward into 5.15 and master. I can help out with that.

Please do :D I find git confusing enough as it is :D

rooty added a comment.Fri, Feb 8, 3:48 PM

Can I use Kai's suggestion to git checkout Plasma/5.12 then commit then checkout Plasma/5.15 then merge Plasma/5.12, then checkout master then merge Plasma/5.15? Will this work if I cloned it off of anongit.kde.org (and not github.com)?

ngraham added a comment.EditedFri, Feb 8, 3:53 PM

Regardless, let's wait for Eike's approval before landing this.

Here's how I would do it, if and when it's time to land this patch:

git checkout Plasma/5.12
git cherry-pick 2c3b1ee1995692aaa68e4d3c76e9bf37565741b2
git push
git checkout Plasma/5.14 (we're putting fixes on 5.14 too since we may ship another 5.14 bugfix release)
git merge Plasma/5.12
git push
git checkout Plasma/5.15
git merge Plasma/5.14
git push
git checkout master
git merge Plasma/5.15
git push
git branch -D fix-kickoff-initial-aftersearch
rooty added a comment.Fri, Feb 8, 3:55 PM

Wow thanks!
And after all is said and done, there's no need to run arc land?

Nah, arc land is just a fancy way of saying "take the commits on this branch and commit them onto the parent branch and then push the parent branch and then delete this branch"

You can always just do those things yourself if you'd like, which the above instructions show you how to do in a different way.

rooty added a comment.Fri, Feb 8, 3:59 PM

Nah, arc land is just a fancy way of saying "take the commits on this branch and commit them onto the parent branch and then push the parent branch and then delete this branch"

You can always just do those things yourself if you'd like, which the above instructions show you how to do in a different way.

I was just wondering about running arc land after the last push because it closes the diff? Or does git push do that too?

The diff is automatically closed once Phab detects a commit associated with it, so you don't have to do that manually (most of the time at least; sometimes it's a tad buggy).

rooty added a comment.Fri, Feb 8, 4:01 PM

The diff is automatically closed once Phab detects a commit associated with it, so you don't have to do that manually (most of the time at least; sometimes it's a tad buggy).

Excellent :D

hein added a comment.Fri, Feb 8, 4:11 PM
In D18848#407838, @hein wrote:

This is a good change, but it's more worth finding out why it doesn't work already. Looking at the code, when KickoffItem is triggered it runs the switchToInitial() function in FullRepresentation.qml, which already switches tabs and is also what sets the state back to Normal. So setting the tab in the state shouldn't be necessary. Can you debug why setting currentTab in switchToInitial() apparently doesn't work?

I think it might be because of the switch from the state "Search" to the state "Normal", because this doesn't happen if the arrow keys are used to navigate to the right of Favorites, only when Search is invoked. There's a "currentTab: searchPage" (line 738) setting in the state "Search" that probably gets stuck. I didn't want to deal with changing "Search" because in my experience, Kickoff's search view can be pretty temperamental. This seemed like the safest course of action.

Doesn't that mean the code in switchToInitial just needs reordering to set the state (to Normal) first and then do the tab change? (Or alternatively the tab change could be cleaned away from the function.)

rooty added a comment.Fri, Feb 8, 4:47 PM

Doesn't that mean the code in switchToInitial just needs reordering to set the state (to Normal) first and then do the tab change? (Or alternatively the tab change could be cleaned away from the function.)

You're right! That's a lot simpler than adding a property change, and it works.

rooty updated this revision to Diff 51197.Fri, Feb 8, 4:49 PM

Reorder code instead of adding property changes

hein accepted this revision.Fri, Feb 8, 6:13 PM
This revision was automatically updated to reflect the committed changes.