Bug 432093 - KCM title is displayed above the sidebar
Summary: KCM title is displayed above the sidebar
Status: RESOLVED FIXED
Alias: None
Product: systemsettings
Classification: Applications
Component: iconview (show other bugs)
Version: 5.23.5
Platform: Arch Linux Linux
: LO normal
Target Milestone: ---
Assignee: Plasma Bugs List
URL:
Keywords: regression
: 436233 442960 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-01-25 12:41 UTC by Patrick Silva
Modified: 2022-05-19 21:06 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.24.90


Attachments
screen recording (1.89 MB, video/webm)
2021-01-25 12:41 UTC, Patrick Silva
Details
Revisited page still with issues (136.12 KB, image/png)
2021-11-11 04:26 UTC, EDeadLock
Details
Normal page (199.94 KB, image/png)
2021-11-11 04:26 UTC, EDeadLock
Details
Header Forced Hidden (105.21 KB, image/png)
2022-01-28 05:53 UTC, EDeadLock
Details
header_hidden.png (105.21 KB, image/png)
2022-01-28 05:54 UTC, EDeadLock
Details
headers_shown.png (139.41 KB, image/png)
2022-01-28 05:55 UTC, EDeadLock
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Silva 2021-01-25 12:41:58 UTC
Created attachment 135163 [details]
screen recording

STEPS TO REPRODUCE
1. set System Settings to Icon View mode
2. click on Workspace Behavior
3. repeatedly switch between all KCMs under Workspace Behavior

OBSERVED RESULT
At some point, the title of the current KCM will be displayed above the sidebar.
Watch the attached screen recording please,

EXPECTED RESULT
Titles of KCMs should always be displayed in their correct position

SOFTWARE/OS VERSIONS
Operating System: Arch Linux
KDE Plasma Version: 5.20.90
KDE Frameworks Version: 5.78.0
Qt Version: 5.15.2
Comment 1 Patrick Silva 2021-05-31 02:49:26 UTC
*** Bug 436233 has been marked as a duplicate of this bug. ***
Comment 2 Nate Graham 2021-09-27 19:53:20 UTC
*** Bug 442960 has been marked as a duplicate of this bug. ***
Comment 3 Nate Graham 2021-09-28 03:49:34 UTC
Bharadwaj, could this have been caused by your changes to speed up KCM loading in sidebar mode?
Comment 4 Bharadwaj Raju 2021-09-28 05:07:25 UTC
(In reply to Nate Graham from comment #3)
> Bharadwaj, could this have been caused by your changes to speed up KCM
> loading in sidebar mode?

I noticed that when working on it, but the issue remained even when I reverted the changes.

Plus, look at the date this was reported :)
Comment 5 Nate Graham 2021-09-28 13:18:57 UTC
derp
Comment 6 EDeadLock 2021-11-11 04:26:06 UTC
Created attachment 143437 [details]
Revisited page still with issues
Comment 7 EDeadLock 2021-11-11 04:26:35 UTC
Created attachment 143438 [details]
Normal page
Comment 8 EDeadLock 2021-11-11 04:26:55 UTC
I would like to add that this issue isnt random and that it also shows up in other settings items other than Workspace Behavior. 

The issue only appears the first time you "visit" a settings page (e.g. Screen Edges page). Each time you revisit the page (by clicking on a different page and then going back to the original page) issue doesnt show up. To get the page to show the issue again you have to either go back to the main settings page or exit and reopen the settings app altogether.

Something else I noticed. After revisiting a page and making the issue go away. It doesn't really go away. Well, it does go away but it shows a different issue. This is better explained with the screenshots I provided. On the tittles there are supposed to be a line that covers the entire page. The padding is not the same as the working pages (You will notice the working page titles are slightly more centered). And I also think the font is slightly smaller? but I cant confirm that

I would love to help fix this issue but I dont know where to start. I managed to set up the neon docker container. How can I enable logging on the settings app or edit the code and test?

One thing that I can try to do is going back to earlier versions because I know this issue wasnt here at some point (i cant remember when though lol or maybe im  imagining things)

As mentioned earlier, this issue also shows up in other settings items and under specific pages. Here is a painstakingly long list:
Workspace Behavior:
* Screen Edges
* Touch Screen
* Activities
Window Management:
* Window Behavior
* Task Switcher
* KWin Scripts
Search:
* Krunner
* Web Search Keywords
Regional Settings:
* Spell Check
* Date & Time
Applications:
* File Associations
* Locations
Settings: (Side rant: "Settings" should reeeeeeally be renamed to something else like "Advanced Network Settings")
* Connection Preferences
* SSL Preferences
* Cookies
Input devices:
* Keyboard
* Touchpad
Display and Monitor:
* Compositor
Power Management: (This one would be a good place to start debugging as all the pages here have the issue)
* Energy Savings
* Active Power Settings
* Advanced Power Settings
Removable Storage:
* Removable Devices
Comment 9 Bharadwaj Raju 2022-01-28 03:04:02 UTC
(In reply to EDeadLock from comment #8)
> I would love to help fix this issue but I dont know where to start. I
> managed to set up the neon docker container. How can I enable logging on the
> settings app or edit the code and test?

Sorry for the late reply. Please check out https://community.kde.org/Get_Involved/development which will help you about compiling the code etc.
Comment 10 EDeadLock 2022-01-28 05:53:12 UTC
Created attachment 146008 [details]
Header Forced Hidden
Comment 11 EDeadLock 2022-01-28 05:54:19 UTC
Created attachment 146009 [details]
header_hidden.png
Comment 12 EDeadLock 2022-01-28 05:55:48 UTC
Created attachment 146010 [details]
headers_shown.png
Comment 13 EDeadLock 2022-01-28 06:01:55 UTC
(In reply to Bharadwaj Raju from comment #9)
> (In reply to EDeadLock from comment #8)
> > I would love to help fix this issue but I dont know where to start. I
> > managed to set up the neon docker container. How can I enable logging on the
> > settings app or edit the code and test?
> 
> Sorry for the late reply. Please check out
> https://community.kde.org/Get_Involved/development which will help you about
> compiling the code etc.

TL;DR;
I was able to remove the incorrect headers using the patch below, but I have yet to figure out how display the correct Header


Yeah, no worries, I was able to figure out how to play with it. Your link and the Docker container setup https://community.kde.org/Neon/Docker came in really helpful in debugging this

I had done some troubleshooting by changing different parts of the code and seeing how things behaved. It's not the best way to go about it but there is really not much else I can do when I am unfamiliar with the code base.

In any case,  I had done the troubleshooting late November and wanted to do some more before I updated here, but I had not found the time to do more, so I will just update what I have with my notes from then. I also added some details from what I remember and also sat down and spent 2 hours investigating more when I originally wanted to only spend 30mins :)


What I have found is that there are 3 Headers within the entire code base. 2 Of those headers are "incorrect" and one is "correct". Pages that use the correct headers have no issues.

I tweaked the code to make it so that all 3 headers are shown at the same time. Well, I found how to make Header1 and Header2 to always be shown or hidden. I havent found the code for Header3. See screenshot headers_shown.png and headers_hidden.png to understand which headers are Header1, Header2, and Header3. Header1 and Header2 are the incorrect headers while Header3 is the correct one

The following diff removes the incorrect Headers Header1 and Header2. To make them always visible just toggle them to true. This is how I made the screenshots. Note that I am on an old commit 2dcc904bcc

diff --git a/core/ModuleView.cpp b/core/ModuleView.cpp
index 203b6021..9d10d22b 100644
--- a/core/ModuleView.cpp
+++ b/core/ModuleView.cpp
@@ -458,6 +458,8 @@ void ModuleView::activeModuleChanged(KPageWidgetItem *current, KPageWidgetItem *
 
         moduleShowDefaultsIndicators(d->mDefaultsIndicatorsVisible);
     }
+    current->setHeaderVisible(false);
+    d->mCustomHeader->setVisible(false);
 }
 
 void ModuleView::stateChanged()
@@ -485,7 +487,7 @@ void ModuleView::stateChanged()
     updatePageIconHeader(d->mPageWidget->currentPage());
 
     KCModuleProxy *moduleProxy = d->mPages.value(d->mPageWidget->currentPage());
-    d->mCustomHeader->setVisible(!moduleProxy || !moduleProxy->realModule()->inherits("KCModuleQml"));
+    d->mCustomHeader->setVisible(false);
 
     d->mApplyAuthorize->setAuthAction(moduleAction);
     d->mDefault->setEnabled(!defaulted);


Header1 is fixed by d->mCustomHeader->setVisible(false);
Header2 is fixed by current->setHeaderVisible(false);

I havent found how to make Header3 present in the broken pages. I guess that is the next step. It seems that all the broken pages seem to be using Header1 and Header2, which are wrong. The pages need to migrate to using Header3 type. It also seems that correct pages use Header3 and stay away from using Header1 and Header2. If anyone knows the trick to display Header3 then we can get started in setting a proper patch

Thanks all :)
Comment 14 Michael 2022-01-28 22:09:57 UTC
Great to see this bug getting some love :)
Comment 15 Bug Janitor Service 2022-01-31 22:26:40 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/systemsettings/-/merge_requests/120
Comment 16 Ismael Asensio 2022-01-31 22:36:00 UTC
(In reply to EDeadLock from comment #13)

> @@ -485,7 +487,7 @@ void ModuleView::stateChanged()
>      updatePageIconHeader(d->mPageWidget->currentPage());
>  
>      KCModuleProxy *moduleProxy =
> d->mPages.value(d->mPageWidget->currentPage());
> -    d->mCustomHeader->setVisible(!moduleProxy ||
> !moduleProxy->realModule()->inherits("KCModuleQml"));
> +    d->mCustomHeader->setVisible(false);

Hi there! You were right in this line causing the bug.

The reason you couldn't find "Header3" is because it is generated in QML by modules that have already been ported to use that technology. There is code to treat the three different cases (QML modules, QWidget modules when in Sidebar View and QWidget modules when in Icon View), but that code wasn't always being run, causing the bug.

The code in this class in general has been suffering from small incremental changes which made it a little too un-manageable, so fixing one thing could break a different one. I hope with a little refactor this is sorted out and not many new surprises arise.
Comment 17 Nate Graham 2022-02-01 15:42:13 UTC
Git commit f92b8fb2dc2e8f8de90502e279a3bf7e842a4bac by Nate Graham, on behalf of Ismael Asensio.
Committed on 01/02/2022 at 15:42.
Pushed by ngraham into branch 'master'.

ModuleView: Improve code for header updates

Clean-up and refactor around updatePageIconHeader() method and uses.

By moving the header logic into this method, we reduce some duplicity
present in different codepaths and make it less bug-prone, specially
on IconView.

M  +39   -60   core/ModuleView.cpp
M  +1    -1    core/ModuleView.h

https://invent.kde.org/plasma/systemsettings/commit/f92b8fb2dc2e8f8de90502e279a3bf7e842a4bac
Comment 18 Nate Graham 2022-02-01 15:43:02 UTC
Git commit 19f88c428de426a8a4db2b43f51f558437bf279e by Nate Graham, on behalf of Ismael Asensio.
Committed on 01/02/2022 at 15:42.
Pushed by ngraham into branch 'Plasma/5.24'.

ModuleView: Improve code for header updates

Clean-up and refactor around updatePageIconHeader() method and uses.

By moving the header logic into this method, we reduce some duplicity
present in different codepaths and make it less bug-prone, specially
on IconView.
FIXED-IN: 5.24


(cherry picked from commit c8a128c0f30dec32e807043590673b9ca70ce0db)

M  +39   -60   core/ModuleView.cpp
M  +1    -1    core/ModuleView.h

https://invent.kde.org/plasma/systemsettings/commit/19f88c428de426a8a4db2b43f51f558437bf279e
Comment 19 Nate Graham 2022-02-01 15:44:09 UTC
Not fully fixed yet; another MR is still needed. Should be done soon.
Comment 20 EDeadLock 2022-02-03 04:23:39 UTC
(In reply to Ismael Asensio from comment #16)
> (In reply to EDeadLock from comment #13)
> 
> > @@ -485,7 +487,7 @@ void ModuleView::stateChanged()
> >      updatePageIconHeader(d->mPageWidget->currentPage());
> >  
> >      KCModuleProxy *moduleProxy =
> > d->mPages.value(d->mPageWidget->currentPage());
> > -    d->mCustomHeader->setVisible(!moduleProxy ||
> > !moduleProxy->realModule()->inherits("KCModuleQml"));
> > +    d->mCustomHeader->setVisible(false);
> 
> Hi there! You were right in this line causing the bug.
> 
> The reason you couldn't find "Header3" is because it is generated in QML by
> modules that have already been ported to use that technology.
Does this mean we have to port/update the modules to use this technology so that QML generates the correct headers?


 There is code
> to treat the three different cases (QML modules, QWidget modules when in
> Sidebar View and QWidget modules when in Icon View), but that code wasn't
> always being run, causing the bug.
> 
> The code in this class in general has been suffering from small incremental
> changes which made it a little too un-manageable, so fixing one thing could
> break a different one. I hope with a little refactor this is sorted out and
> not many new surprises arise.

Thank you Ismael for the explanation.

I was able to test latest master (06df6b61) and I see that one of the bad headers is gone (Header1) but we still have Header2 present and the correct header3 is not yet shown. Small step in the right direction, again, thank you so much. I assume we have to port/update the modules to use the newer technology so that we get the correct header shown as I asked above. Anyway I can help with this? Please let me know what I can do to help, best!
Comment 21 Patrick Silva 2022-05-19 19:47:32 UTC
Cannot reproduce on Plasma 5.25 beta.

Operating System: Arch Linux
KDE Plasma Version: 5.24.90
KDE Frameworks Version: 5.94.0
Qt Version: 5.15.4
Graphics Platform: Wayland
Comment 22 Ismael Asensio 2022-05-19 20:34:04 UTC
> Cannot reproduce on Plasma 5.25 beta.

I think this got fixed with https://invent.kde.org/plasma/systemsettings/-/merge_requests/130, but as I was trying to fix a different issue, I didn't remember to reference it to this bug.

Thanks for reminding it!