Allow 'Exclude folders' section in Baloo KCM to fill window
ClosedPublic

Authored by kishoreg on May 26 2019, 1:33 PM.

Details

Summary

Initially, there was a spacer which took up a significant amount of space at the bottom of the window without any apprent justification.

I removed a spacer to allow the folder list to occupy the available space.

It seems like it was added intentionally, so I'm not sure if removing it breaks something. I couldn't find any broken behaviour with this change.

BUG: 407709

Test Plan
  1. Open kcm_baloofile. Check if the exclude folders section fills all available space.
  2. Resize the window to make it larger. Check that the exclude folders section resizes to match the window.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
kishoreg created this revision.May 26 2019, 1:33 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 26 2019, 1:33 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kishoreg requested review of this revision.May 26 2019, 1:33 PM
bruns added a subscriber: bruns.May 26 2019, 7:53 PM

The code change looks sane for me, although I haven't tested it.

The summary should mention:

  • what is currently wrong (space consumed by spacer)
  • what was done to fix it (spacer removed, so the folder list can stretch)

Can you add a before/after screenshot here (the one from the BR is fine for before)?

The config layout was imported (more or less) as is 4 years ago, maybe inherited from nepomuk. Hard to tell if the spacer ever had a purpose.

Tested; LGTM. While you're cleaning up the UI, you could probably remove the group box too, and just add a left-aligned label above the list view. What do you think?

Before change:


Initial patch:

With box removed:

kishoreg updated this revision to Diff 58723.May 27 2019, 1:42 PM
kishoreg edited the summary of this revision. (Show Details)

This patch incorporates Nate's suggestion to remove the box around the 'exclude folders' area. I've added screenshots of all three cases (with spacer, with box, and without box) to a previous comment.

Great! I think a bit of padding between the checkbox and the label would help too. Then this'll be a great UI upgrade!

kcms/baloo/configwidget.ui
60

Now this string needs to end with a colon

Do not search in these locations:

kishoreg updated this revision to Diff 58781.May 28 2019, 12:36 PM

I've added a colon after the 'Do not search in these locations' text. I've also added spacing between the checkboxes and their text. I've put in 10 px for now. Is there a better way to do this?

Without additional space after checkbox:

With additional space after checkbox:

The change seems to have slightly reduced the vertical space between the checkboxes, so should I add padding to the stylesheet?

kishoreg marked an inline comment as done.May 28 2019, 12:44 PM

BTW, checkboxes in other KCMs also seem to have the same spacing as this one had before this change. Is there a way to globally apply this spacing / padding consistently to QCheckBoxes in all KCMs for the sake of consistency? Perhaps some sort of global stylesheet that this should go into?

kishoreg updated this revision to Diff 58784.May 28 2019, 12:55 PM

I seem to have messed up the indentation in my previous diff. Here it is with the indentation fixed.

kishoreg updated this revision to Diff 58786.May 28 2019, 1:07 PM

This is awkward. I hadn't realized that I was supposed to use one space for indentation. Fixed as per the preexisting convention.

I think I confused you, sorry. My desire was for some spacing above the listview's label, like this:

kishoreg updated this revision to Diff 58797.May 28 2019, 4:34 PM

Screenshot as of this diff:

Are there any other changes that need to be made?

Nope, that looks perfect to me!

Actually now that I think about it, this should probably go into Plasma 5.16. However since I asked you to add a colon to the end of a string, technically that's a string change and those aren't allowed so close to the release, so it would have to go into Plasma 5.17

I propose reverting that part so there are no string changes, and then this can go into 5.16. Then let's add the colon back on the master branch. How does that sound?

GB_2 added a subscriber: GB_2.May 28 2019, 4:40 PM

Actually now that I think about it, this should probably go into Plasma 5.16. However since I asked you to add a colon to the end of a string, technically that's a string change and those aren't allowed so close to the release, so it would have to go into Plasma 5.17

I propose reverting that part so there are no string changes, and then this can go into 5.16. Then let's add the colon back on the master branch. How does that sound?

Yeah. Can you also please move the buttons to the left?

BTW, if I open the window standalone (from krunner), it allows me to resize it vertically so that the entire window gets scrollbars.


I can't do this horizontally. Is there some vertical minimum size hint I have to set for the entire window? The minimum size is obeyed correctly when we open it as part of the System Settings window. Is this intended behaviour or a known bug? The result is a bit awkward to use, with scrollbars within scrollbars.

In D21414#471286, @GB_2 wrote:

Actually now that I think about it, this should probably go into Plasma 5.16. However since I asked you to add a colon to the end of a string, technically that's a string change and those aren't allowed so close to the release, so it would have to go into Plasma 5.17

I propose reverting that part so there are no string changes, and then this can go into 5.16. Then let's add the colon back on the master branch. How does that sound?

Yeah. Can you also please move the buttons to the left?

Oh right, I forgot about that, Yeah. That will make this align with T10384.

BTW, if I open the window standalone (from krunner), it allows me to resize it vertically so that the entire window gets scrollbars.


I can't do this horizontally. Is there some vertical minimum size hint I have to set for the entire window? The minimum size is obeyed correctly when we open it as part of the System Settings window. Is this intended behaviour or a known bug? The result is a bit awkward to use, with scrollbars within scrollbars.

That means the SizeHint is probably not set correctly to account for the new layout.

kishoreg updated this revision to Diff 58838.May 29 2019, 12:42 PM
kishoreg edited the test plan for this revision. (Show Details)

Moved buttons to the left.

I'll try to figure out how to set the size hint properly.

With no items in the list view, the size looks perfect to me:

Are you seeing something different?

With no items in the list view, the size looks perfect to me:

Are you seeing something different?

The initial size when it opens is set correctly. However, it doesn't seem to properly set a vertical minimum size, so one is able to make the window small enough that some of the widgets are accessible only by scrolling. I don't have any window rules that might apply to this window. I can also reproduce this in another user.

Initial size set correctly:

Minimum size while resizing is not set properly:

If the initial size is fine, that's good enough. We don't need to prevent people from resizing the window to be really small.

If you remove the colon from the string (see earlier discussion), I think this could land in 5.16!

kishoreg updated this revision to Diff 58849.May 29 2019, 2:45 PM
kishoreg edited the test plan for this revision. (Show Details)

Removed colon to keep string unchanged.

Screenshot:

ngraham accepted this revision.May 29 2019, 2:56 PM

Thanks so much. I'll land this on the 5.16 branch.

This revision is now accepted and ready to land.May 29 2019, 2:56 PM
This revision was automatically updated to reflect the committed changes.