D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Arjen Hiemstra]

ahiemstra abandoned this revision.
ahiemstra added a comment.


  > The bug mentioned got fixed by this (old Qt doesn't iterate over QList).
  > 
  > I don't see how this would make it look any different. The kirigami visible 
patch is indeed necessary but for an entirely different reason.
  
  You're right, it does seem to work correctly now. Weird, when I made this I 
was sure the problem was with the sizing of the ActionToolBar. But maybe I was 
put off by the overflow button visibility.

REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: rikmills, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Nathaniel Graham]

ngraham added a comment.


  Wish I could verify, but the latest commit causes Discover to crash when 
visiting the Settings page for me: https://bugs.kde.org/show_bug.cgi?id=415727

REPOSITORY
  R134 Discover Software Store

BRANCH
  sources_actiontoolbar

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: rikmills, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Aleix Pol Gonzalez]

apol added a comment.


  F7851481: Screenshot_20191231_024108.png 

  
  The bug mentioned got fixed by this (old Qt doesn't iterate over QList).
  
  I don't see how this would make it look any different. The kirigami visible 
patch is indeed necessary but for an entirely different reason.

REPOSITORY
  R134 Discover Software Store

BRANCH
  sources_actiontoolbar

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: rikmills, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Aleix Pol Gonzalez]

apol added a comment.


  This feels workaround-y, maybe we can just fix ListSectionHeader?

REPOSITORY
  R134 Discover Software Store

BRANCH
  sources_actiontoolbar

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: rikmills, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Rik Mills]

rikmills added a comment.


  This does NOT fix BUG: 415666. The distro native sources button is still 
missing.
  
  Git master with this change:
  
  F7850103: 230501812c.png 
  
  Correctly showing sources button before commit 
https://cgit.kde.org/discover.git/commit/?id=89088f7639a63c0c099ccb9c95393f12d83c17b5
  
  F7850101: 5e38cc3bf6.png 

REPOSITORY
  R134 Discover Software Store

BRANCH
  sources_actiontoolbar

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: rikmills, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Ah my mistake. I was confused by the empty overflow issue, which your other 
patch fixes. I can confirm that this fixes the actual reported issue. :)
  
  However... I wonder if it's also worth fixing ListSectionHeader. It is indeed 
designed for you to feed it items rather than building your own custom layout, 
as this was perceived as being more simple. However I can see how this doesn't 
work at all when you put an ActionToolbar in it since it winds up inside the 
Rowlayout.  I wonder if ultimately the idea was bad and we should just require 
that you give it your own custom layout... or if maybe it can detect when  the 
item you give it is or has a layout and then replace the RowLayout with it? Or 
maybe that's a terrible idea.

REPOSITORY
  R134 Discover Software Store

BRANCH
  sources_actiontoolbar

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Arjen Hiemstra]

ahiemstra added a comment.


  I've created https://phabricator.kde.org/D26279 for the overflow menu.
  
  I understood this bug to be about certain buttons now being hidden on the 
sources page, which as far as I could tell was happening due to sizing, which 
is what this patch fixes.

REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma, #discover_software_store, apol
Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Nathaniel Graham]

ngraham added a comment.


  Hmm, it doesn't seem to fix the issue for me: F7849179: 
Screenshot_20191229_101410.png 
  
  FWIW I've noticed the same phantom overflow menu on the Application page's 
toolbar: F7849182: Screenshot_20191229_101453.png 


REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D26275

To: ahiemstra, #plasma
Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Arjen Hiemstra]

ahiemstra created this revision.
ahiemstra added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ahiemstra requested review of this revision.

REVISION SUMMARY
  Kirigami's ListSectionHeader has a default property that points to an 
embedded RowLayout's
  data property, in an attempt (I guess?) to make it simpler to add things to 
it. However,
  this does not work properly, at least not in combination with ActionToolBar. 
So instead,
  override the section header's contentItem so we can specify the right layout 
properties.
  
  BUG: 415666

TEST PLAN
  The sources page now displays actions properly, but still places them in the 
overflow menu
  when the window gets smaller.

REPOSITORY
  R134 Discover Software Store

BRANCH
  sources_actiontoolbar

REVISION DETAIL
  https://phabricator.kde.org/D26275

AFFECTED FILES
  discover/qml/SourcesPage.qml

To: ahiemstra, #plasma
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart