TDE
/
tdebase
9
4
Fork 1
Code Issues 115 Pull Requests 10 Releases Wiki Activity
Labels Milestones

Introduce additional sorting / grouping options and actions for Konqueror Listviews (split from PR #196) #240

Merged
MicheleC merged 1 commits from feature/konq-listview-enhancements into master 2 years ago
Conversation 12 Commits 1 Files Changed 10
VinceR commented 2 years ago
Collaborator

New Konqueror listview options & actions, available through new submenu: View => Sort

Options:

(1) "Group Directories First" - Toggle on/off option to group directories
before non-directories. Comparable to iconview's "Folders First" option.

(2) "Group Hidden First" - Toggle on/off option to group hidden entities
(aka dotfiles) before non-hidden.

Option-related settings are stored in config/konqlistviewrc

Actions:

(1) "Reverse Sort Order" - Toggle sort order of the current sort column.
Action bound to key Ctrl+R.

(2) "Alternate Sort Order" - Toggle sorting between 2 most recent sort
columns selected by mouse click. The existing sort order for the
sort columns is preserved. Action bound to key Ctrl+S.

Action-related settings are stored in config/konquerorrc/[Listview_file]

Signed-off-by: Vincent Reher tde@4reher.org

New Konqueror listview options & actions, available through new submenu: View => Sort Options: (1) "Group Directories First" - Toggle on/off option to group directories before non-directories. Comparable to iconview's "Folders First" option. (2) "Group Hidden First" - Toggle on/off option to group hidden entities (aka dotfiles) before non-hidden. Option-related settings are stored in config/konqlistviewrc Actions: (1) "Reverse Sort Order" - Toggle sort order of the current sort column. Action bound to key Ctrl+R. (2) "Alternate Sort Order" - Toggle sorting between 2 most recent sort columns selected by mouse click. The existing sort order for the sort columns is preserved. Action bound to key Ctrl+S. Action-related settings are stored in config/konquerorrc/[Listview_file] Signed-off-by: Vincent Reher <tde@4reher.org>
VinceR changed title from WIP: Introduce additional sorting / grouping options and actions for Konqueror (split from PR #196) to WIP: Introduce additional sorting / grouping options and actions for Konqueror Listviews (split from PR #196) 2 years ago
MicheleC commented 2 years ago
Owner

Thanks @VinceR,
I will check this within the end of this week 😄

Thanks @VinceR, I will check this within the end of this week :smile:
MicheleC requested changes 2 years ago
MicheleC left a comment
Owner

Please amend based on feedback below.

Please amend based on feedback below.
konqueror/listview/konq_listview.cpp Outdated
m_sortColumnOrderAlternate = config.alternateSortOrder();
if (m_sortColumnIndexPrimary >= 0 && m_sortColumnIndexAlternate >= 0)
if (m_sortColumnIndexPrimary != m_sortColumnIndexAlternate)
MicheleC commented 2 years ago
Owner

I don't think we need the second if (line 683). As long as two columns have been defined, it should be fine.

I don't think we need the second if (line 683). As long as two columns have been defined, it should be fine.
VinceR commented 2 years ago
Poster
Collaborator

I was a little concerned about this but testing did not reveal a problem. I also removed a couple of pointless variable declarations at start of function.

I was a little concerned about this but testing did not reveal a problem. I also removed a couple of pointless variable declarations at start of function.
MicheleC marked this conversation as resolved
konqueror/listview/konq_listview.cpp Outdated
if (m_sortColumnIndexPrimary >= 0 && m_sortColumnIndexAlternate >= 0)
if (m_sortColumnIndexPrimary != m_sortColumnIndexAlternate)
return ;
// FIXME: should we check other values?
MicheleC commented 2 years ago
Owner

Looks fine, we can remove FIXME comment (line 685)

Looks fine, we can remove FIXME comment (line 685)
MicheleC marked this conversation as resolved
konqueror/listview/konq_listview.h Outdated
{
friend class KonqBaseListViewWidget;
friend class ListViewBrowserExtension;
friend void KonqListViewItem::updateContents();
MicheleC commented 2 years ago
Owner

This (line 66) is not required, see comment on file konqueror/listview/konq_listviewitems.cpp.

This (line 66) is not required, see comment on file konqueror/listview/konq_listviewitems.cpp.
MicheleC marked this conversation as resolved
konqueror/listview/konq_listview.h Outdated
friend class KonqBaseListViewWidget;
friend class ListViewBrowserExtension;
friend void KonqListViewItem::updateContents();
friend int KonqBaseListViewItem::compare(TQListViewItem* item, int col, bool ascending) const;
MicheleC commented 2 years ago
Owner

Line 67 (KonqBaseListViewItem::compare) not required for this PR.

Line 67 (KonqBaseListViewItem::compare) not required for this PR.
MicheleC marked this conversation as resolved
konqueror/listview/konq_listview.kcfg Outdated
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.kde.org/standards/kcfg/1.0
http://www.kde.org/standards/kcfg/1.0/kcfg.xsd" >
http://www.kde.org/standards/kcfg/1.0/kcfg.xsd" >
MicheleC commented 2 years ago
Owner

line 5: original indentation is actually preferred.

line 5: original indentation is actually preferred.
MicheleC marked this conversation as resolved
MicheleC reviewed 2 years ago
konqueror/listview/konq_listviewitems.cpp Outdated
sortChar = S_ISDIR( m_fileitem->mode() ) ? 1 : 3;
if ( m_fileitem->text()[0] == '.' )
--sortChar;
bool m_groupDirectoriesFirst = m_pListViewWidget->m_pBrowserView->m_pProps->isDirsFirst();
MicheleC commented 2 years ago
Owner

Please replace line 87-88 with

   bool m_groupDirectoriesFirst = m_pListViewWidget->props()->isDirsFirst();
   bool m_groupHiddenFirst      = m_pListViewWidget->props()->isHiddenFirst();

This avoids the need for the 'friend' in konqueror/listview/konq_listview.h:66

Please replace line 87-88 with ``` bool m_groupDirectoriesFirst = m_pListViewWidget->props()->isDirsFirst(); bool m_groupHiddenFirst = m_pListViewWidget->props()->isHiddenFirst(); ``` This avoids the need for the 'friend' in konqueror/listview/konq_listview.h:66
MicheleC marked this conversation as resolved
MicheleC commented 2 years ago
Owner

@VinceR
thanks for fixing up the points I had raised last week.
PR looks good now. Before merging, I will ask @SlavekB to have a look too, given it is a change in functionality and API too.
Code will go into R14.1.0 but not in R14.0.x most likely.

@VinceR thanks for fixing up the points I had raised last week. PR looks good now. Before merging, I will ask @SlavekB to have a look too, given it is a change in functionality and API too. Code will go into R14.1.0 but not in R14.0.x most likely.
MicheleC requested review from SlavekB 2 years ago
MicheleC approved these changes 2 years ago
MicheleC left a comment
Owner

Looks good now.

Looks good now.
SlavekB requested changes 2 years ago
SlavekB left a comment
Owner

It looks good, but there are some little things that would be good to do.
See comments below.

It looks good, but there are some little things that would be good to do. See comments below.
konqueror/listview/konq_listview.cpp Outdated
columnNumber = -1;
for (uint i=0; i<m_pListView->NumberOfAtoms; i++)
if (m_pListView->confColumns[i].displayInColumn==defaultVisibleColumn)
columnNumber=i;
SlavekB commented 2 years ago
Owner

Omitting parentheses in this section causes poor code readability.

Omitting parentheses in this section causes poor code readability.
MicheleC commented 2 years ago
Owner

Good catch, adding braces ({ }) will help. As well as adding spaces around < and == and =.

Good catch, adding braces ({ }) will help. As well as adding spaces around < and == and =.
MicheleC marked this conversation as resolved
konqueror/listview/konq_listview.cpp Outdated
columnNumber=-1;
for (uint i=0; i<m_pListView->NumberOfAtoms; i++)
if (m_pListView->confColumns[i].displayInColumn==defaultVisibleColumn)
columnNumber=i;
SlavekB commented 2 years ago
Owner

Omitting parentheses in this section causes poor code readability.

Omitting parentheses in this section causes poor code readability.
MicheleC commented 2 years ago
Owner

Same as above about spaces and braces.

Same as above about spaces and braces.
VinceR commented 2 years ago
Poster
Collaborator

I got a bit confused about parentheses () versus braces {}. Is this what you are looking for:

columnNumber = -1;
for ( uint i = 0; i < m_pListView->NumberOfAtoms; i++ ) {
  if ( m_pListView->confColumns[i].displayInColumn == defaultVisibleColumn )
    columnNumber = i;
}

... or is there another opportunity to introduce () that I missed?

I got a bit confused about parentheses () versus braces {}. Is this what you are looking for: ``` columnNumber = -1; for ( uint i = 0; i < m_pListView->NumberOfAtoms; i++ ) { if ( m_pListView->confColumns[i].displayInColumn == defaultVisibleColumn ) columnNumber = i; } ``` ... or is there another opportunity to introduce () that I missed?
VinceR commented 2 years ago
Poster
Collaborator

(infer lost indentation in above example)

(infer lost indentation in above example)
SlavekB commented 2 years ago
Owner

Yes, I meant braces – {...}. At least for for, but better it would also be for if. You can look at code formatting style.

Yes, I meant braces – `{...}`. At least for `for`, but better it would also be for `if`. You can look at [code formatting style](../tde/wiki/Code-formatting-style-discussion).
VinceR commented 2 years ago
Poster
Collaborator

Yes, I will look at that. I may have picked up some undesirable idioms from years of coding in Perl :)

Yes, I will look at that. I may have picked up some undesirable idioms from years of coding in Perl :)
MicheleC commented 2 years ago
Owner

Basically braces { } around both for and if bodies, allman style ({ on new line) and no space after ( and before )

columnNumber = -1;
for (uint i = 0; i < m_pListView->NumberOfAtoms; i++)
{
  if (m_pListView->confColumns[i].displayInColumn == defaultVisibleColumn)
  {
    columnNumber = i;
  }
}
Basically braces { } around both for and if bodies, allman style ({ on new line) and no space after ( and before ) ``` columnNumber = -1; for (uint i = 0; i < m_pListView->NumberOfAtoms; i++) { if (m_pListView->confColumns[i].displayInColumn == defaultVisibleColumn) { columnNumber = i; } } ```
MicheleC marked this conversation as resolved
libkonq/konq_propsview.cpp Outdated
m_iItemTextPos = config->readNumEntry( "ItemTextPos", TQIconView::Bottom );
d->sortcriterion = config->readEntry( "SortingCriterion", "sort_nci" );
d->dirsfirst = config->readBoolEntry( "SortDirsFirst", true );
d->dirsfirst = config->readBoolEntry( "GroupDirsFirst", true );
SlavekB commented 2 years ago
Owner

Because it changes the name of the configuration variable, here should be created kconf update file to automatically update the existing user configuration.

Because it changes the name of the configuration variable, here should be created _kconf update_ file to automatically update the existing user configuration.
MicheleC commented 2 years ago
Owner

Good point, I missed that. I think there are a couple of easy solutions we can use.

  1. do not change the config name, so keep "SortDirsFirst" instead of "GroupDirsFirst". As well as changing "GroupHiddenFirst" to "SortHiddenFirst". This is the easiest approach.

  2. change the code to read original entry if the new one is not found

d->hiddenfirst = config->readBoolEntry( "GroupHiddenFirst", config->readBoolEntry( "SortDirsFirst", true ) );

This will minimize the required changes. Personally I would go for option 1.
What do you think?

Good point, I missed that. I think there are a couple of easy solutions we can use. 1. do not change the config name, so keep "SortDirsFirst" instead of "GroupDirsFirst". As well as changing "GroupHiddenFirst" to "SortHiddenFirst". This is the easiest approach. 2. change the code to read original entry if the new one is not found ``` d->hiddenfirst = config->readBoolEntry( "GroupHiddenFirst", config->readBoolEntry( "SortDirsFirst", true ) ); ``` This will minimize the required changes. Personally I would go for option 1. What do you think?
VinceR commented 2 years ago
Poster
Collaborator

Somewhere along the line I changed the 'Sort' terminology to 'Group' because I figured that reverse sorting when SortDirsFirst is checked should imply that directories would sort last:)

But that's a quibble and since the Icon view feature came first, I will go with option 1 as you suggest.

Somewhere along the line I changed the 'Sort' terminology to 'Group' because I figured that reverse sorting when SortDirsFirst is checked should imply that directories would sort last:) But that's a quibble and since the Icon view feature came first, I will go with option 1 as you suggest.
MicheleC commented 2 years ago
Owner

Sounds good to me.

Sounds good to me.
MicheleC commented 2 years ago
Owner

I initially meant to rename all "group" to "sort" in option 1. I could be picky and say that now the option says "SortDirsFirst" but the action name still says "Group &Directories First", but I guess no one will ever really look at the config file directly since those options can be enable from the Konqueror menu. If @SlavekB also agrees, we can keep the code as it is now.

I initially meant to rename all "group" to "sort" in option 1. I could be picky and say that now the option says "SortDirsFirst" but the action name still says "Group &Directories First", but I guess no one will ever really look at the config file directly since those options can be enable from the Konqueror menu. If @SlavekB also agrees, we can keep the code as it is now.
SlavekB commented 2 years ago
Owner

Yes, it seems good that the configuration variables related to the sorting are consistently called Sort… and there is no problem that GUI items are named as it seems more concise Group….

Yes, it seems good that the configuration variables related to the sorting are consistently called _Sort…_ and there is no problem that GUI items are named as it seems more concise _Group…_.
SlavekB marked this conversation as resolved
VinceR commented 2 years ago
Poster
Collaborator

The requested changes are simple but unfortunately I have run out of time (leaving on a trip). I need to postpone testing & committing for another week.

The requested changes are simple but unfortunately I have run out of time (leaving on a trip). I need to postpone testing & committing for another week.
MicheleC commented 2 years ago
Owner

The requested changes are simple but unfortunately I have run out of time (leaving on a trip). I need to postpone testing & committing for another week.

No worries Vince, take your time.

> The requested changes are simple but unfortunately I have run out of time (leaving on a trip). I need to postpone testing & committing for another week. No worries Vince, take your time.
MicheleC commented 2 years ago
Owner

@VinceR
thanks. I was about the make the changes myself last Sunday but ultimately decided to wait one more week. And then you pushed the new commit a few hours later. Just nice! 😄

@VinceR thanks. I was about the make the changes myself last Sunday but ultimately decided to wait one more week. And then you pushed the new commit a few hours later. Just nice! :smile:
SlavekB approved these changes 2 years ago
SlavekB left a comment
Owner

Everything seems to be fine.

You don't forget to do rebase to the current head and at the same time seems to be a good idea to make squash of all commit to one and omit comments Code modifications in response to request… 😸

Everything seems to be fine. You don't forget to do rebase to the current head and at the same time seems to be a good idea to make squash of all commit to one and omit comments _Code modifications in response to request…_ 😸
MicheleC commented 2 years ago
Owner

You don't forget to do rebase to the current head and at the same time seems to be a good idea to make squash of all commit to one and omit comments Code modifications in response to request… 😸

@VinceR since your commits are not GPG signed, I will take care of squash and rebase. You will remain the author.

> You don't forget to do rebase to the current head and at the same time seems to be a good idea to make squash of all commit to one and omit comments Code modifications in response to request… 😸 @VinceR since your commits are not GPG signed, I will take care of squash and rebase. You will remain the author.
MicheleC force-pushed feature/konq-listview-enhancements from a53c3375b6 to 9cd504156b 2 years ago
MicheleC changed title from WIP: Introduce additional sorting / grouping options and actions for Konqueror Listviews (split from PR #196) to Introduce additional sorting / grouping options and actions for Konqueror Listviews (split from PR #196) 2 years ago
MicheleC merged commit 9cd504156b into master 2 years ago
MicheleC deleted branch feature/konq-listview-enhancements 2 years ago
MicheleC added this to the R14.1.0 release milestone 2 years ago
MicheleC commented 2 years ago
Owner

@VinceR
I am glad we have finally merged the first part of the original PR #183!!
Well done and thanks for the valuable contribution.
As mentioned earlier, this will go into R14.1.0 but not R14.0.x due to the extensive changes involved.
Now we shall move on on the remaining dictionary sort part (see PR #196).

@VinceR I am glad we have finally merged the first part of the original PR #183!! Well done and thanks for the valuable contribution. As mentioned earlier, this will go into R14.1.0 but not R14.0.x due to the extensive changes involved. Now we shall move on on the remaining dictionary sort part (see PR #196).
VinceR commented 2 years ago
Poster
Collaborator

Thank you, MicheleC and SlavekB, for working with me on this PR. Should we continue dictionary (and version) sort with PR #196 or start a new PR?

Thank you, MicheleC and SlavekB, for working with me on this PR. Should we continue dictionary (and version) sort with PR #196 or start a new PR?
MicheleC commented 2 years ago
Owner

We can continue discussion using PR #196 initially (I posted a comment yesterday) and at some point we will open a PR only for that.

We can continue discussion using PR #196 initially (I posted a comment yesterday) and at some point we will open a PR only for that.
MicheleC commented 1 year ago
Owner

@VinceR
it occoured to me that we didn't add documentation in the Konqueror's handbook about this new functionality. That would be very useful for users.
Would you be able to work on that at some point? Please use a new PR when you want to submit the changes.

@VinceR it occoured to me that we didn't add documentation in the Konqueror's handbook about this new functionality. That would be very useful for users. Would you be able to work on that at some point? Please use a new PR when you want to submit the changes.
VinceR commented 1 year ago
Poster
Collaborator

@VinceR
it occoured to me that we didn't add documentation in the Konqueror's handbook about this new functionality. That would be very useful for users.
Would you be able to work on that at some point? Please use a new PR when you want to submit the changes.

Hi MicheleC,

I didn't see your message until today. I took a look at my Konqueror handbook only to find out I don't have one on my system (will have to research this) -- so I took a look at the XML docbook files in the source code to see what they say.

I think a whole section could be devoted to discussion of:

  1. Controlling which listview columns are displayed
  2. Methods of sorting by column
  3. Sort comparison types
  4. "Group (directories/hidden) first" and its effect on sorting
  5. "Hidden" file filtering
  6. Mysterious but useful file filtering capability in entry box to right of location bar.

I'll try to put together a document of potential verbiage to add to handbook and submit it as a PR in the next month or so. We can discuss, then with your help, I can put together a docbook page.

Vince

> @VinceR > it occoured to me that we didn't add documentation in the Konqueror's handbook about this new functionality. That would be very useful for users. > Would you be able to work on that at some point? Please use a new PR when you want to submit the changes. Hi MicheleC, I didn't see your message until today. I took a look at my Konqueror handbook only to find out I don't have one on my system (will have to research this) -- so I took a look at the XML docbook files in the source code to see what they say. I think a whole section could be devoted to discussion of: 1. Controlling which listview columns are displayed 1. Methods of sorting by column 1. Sort comparison types 1. "Group (directories/hidden) first" and its effect on sorting 1. "Hidden" file filtering 1. Mysterious but useful file filtering capability in entry box to right of location bar. I'll try to put together a document of potential verbiage to add to handbook and submit it as a PR in the next month or so. We can discuss, then with your help, I can put together a docbook page. Vince
MicheleC commented 1 year ago
Owner

Great thanks a lot. We can add that for R14.1.1 :-)

Great thanks a lot. We can add that for R14.1.1 :-)

Reviewers

MicheleC approved these changes 2 years ago
SlavekB approved these changes 2 years ago
The pull request has been merged as 9cd504156b.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
MicheleC
SlavekB
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.1.0 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdebase#240
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feature/konq-listview-enhancements'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes