Proposed resolution for issue # 252 for Konqueror listviews #253

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

CLARIFICATION

Due to a rogue copy/paste, the issue referenced in the original title and commit message for this PR was incorrect. The issue correlated with this PR is #252 and the commit with the corrected message is fd582fb069

NOTES

I implement a new sort option "Ignore Locale Location" which always implements the traditional character code number ordering regardless of current locale collation.

Although I believe that the option "Case Insensitive Sort" is meaningful only when "Ignore Locale Collation" is checked, I do implement it when that option is unchecked, since it does produce slightly different sorting.

I decided that trying to determine whether or not TQString::localeAwareCompare() is "case sensitive" is pointless on the assumption that it always is for all locales. The associated original code is being left alone for now as (mostly) dead code to be removed later. That includes (in libkonq/konq_settings.*):

Variable KonqFMSettingsPrivate.localeAwareCompareIsCaseSensitive
Function: KonqFMSettings::caseSensitiveCompare

I tried but failed to implement a solution for iconviews. I am baffled by that code and how it is distributed between libkonq and konquereror/iconview compared to listviews. Something will need to be done with KFileIVI::compare() in libkonq/tdefileivi.* since that code utilizes the original code.

To be further discussed/reviewed once we have clear understanding on what we want to achieve in #252.

This latest pull has changed this PR from simple issue fix to introduction of new features.

Hi @VinceR,
I finally managed to have a first look at this.

Code wise, there will be several minor adjustment required at some point, but before that I would like to understand why you define 4 sort options in the enum but the menu only contains 3 options.

Build wide, I rebased the PR on top of current master and tried bulding in Debian bookworm. I ran into a FTBFS with the following linking error

[1/1] Linking CXX shared module konqueror/listview/konq_listview.so
/usr/bin/ld: konqueror/listview/CMakeFiles/konq_listview-module.dir/konq_listview.cpp.o: in function `KonqListView::slotSortOptionDialog()':
./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1039: undefined reference to `Sort_Options_Dialog::Sort_Options_Dialog(unsigned short, bool, bool)'
/usr/bin/ld: ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1094: undefined reference to `Sort_Options_Dialog::~Sort_Options_Dialog()'
/usr/bin/ld: ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1094: undefined reference to `Sort_Options_Dialog::~Sort_Options_Dialog()'
collect2: error: ld returned 1 exit status

Last point is about the need or not for a sort dialog option. I know we debated about this on issue #252 and wanted to build this PR to see the result. At the same time, I looked at the Konqueror menu and perhaps it may be wiser to just use the current menu. I suspend my judgement till this PR is buildable and I can compare the two versions. Of course, I will also ask @SlavekB for his opinion at some point.

Sorry and the late reply, it has been (still is) a super busy period and my time for TDE is still limited :-(

Code wise, there will be several minor adjustment required at some point, but before that I would like to understand why you define 4 sort options in the enum but the menu only contains 3 options.

There was no compelling reason to do that, since we only present 3 in the UI. I guess I thought to leave it in the code as a backward reference to how "Case Sensitive" sort is currently implemented, or as a placeholder in case we decide to somehow improve locale-aware sorting.

Build wide, I rebased the PR on top of current master and tried bulding in Debian bookworm. I ran into a FTBFS with the following linking error

[1/1] Linking CXX shared module konqueror/listview/konq_listview.so
/usr/bin/ld: konqueror/listview/CMakeFiles/konq_listview-module.dir/konq_listview.cpp.o: in function `KonqListView::slotSortOptionDialog()':
./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1039: undefined reference to `Sort_Options_Dialog::Sort_Options_Dialog(unsigned short, bool, bool)'
/usr/bin/ld: ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1094: undefined reference to `Sort_Options_Dialog::~Sort_Options_Dialog()'
/usr/bin/ld: ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1094: undefined reference to `Sort_Options_Dialog::~Sort_Options_Dialog()'
collect2: error: ld returned 1 exit status

Just to rule out some stuff: 2 of the build-related files I changed were
libkonq/CMakeLists.txt and libkonq/Makefile.am, so make sure those changes are propagated and make sure cmake is re-run. I use Gentoo portage for building on a "production support" system and here is the cmake command that is run before running make to build libkonq:

cmake -C /scratchfs/var_tmp_portage/portage/trinity-base/libkonq-9999/work/libkonq-9999_build/gentoo_common_config.cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_PREFIX=/usr/trinity/14 -DCMAKE_INSTALL_RPATH=/usr/trinity/14 -DWITH_ARTS=no -DBUILD_LIBKONQ=ON -DBUILD_ALL=OFF -DWITH_ALL_OPTIONS=OFF -DCMAKE_BUILD_TYPE=Gentoo -DCMAKE_TOOLCHAIN_FILE=/scratchfs/var_tmp_portage/portage/trinity-base/libkonq-9999/work/libkonq-9999_build/gentoo_toolchain.cmake -DWITH_GCC_VISIBILITY=OFF  /scratchfs/var_tmp_portage/portage/trinity-base/libkonq-9999/work/libkonq-9999

... but that doesn't look like it provides much insight.

Last point is about the need or not for a sort dialog option. I know we debated about this on issue #252 and wanted to build this PR to see the result. At the same time, I looked at the Konqueror menu and perhaps it may be wiser to just use the current menu. I suspend my judgement till this PR is buildable and I can compare the two versions. Of course, I will also ask @SlavekB for his opinion at some point.

To give you some history: at first I did not see any way of implementing a tri-value TDE menu option, so I decided to tackle this by learning how to create a QT dialog to do the job with radio buttons. And while I was at it, I thought I would put all sort-related option settings in this dialog. Later, I figured that this might depart a bit too much from the usual way of doing things via menu. I then looked harder and discovered there was a non-hacky way of implementing a 3-value menu option. So I did just that and made sure that the two interfaces coexisted properly.

But I understand that this may result in one UI too many. I'm OK if you and @SlavekB decide not to include it. But I will say that <pitch> using my dialog is far faster and simpler than navigating a menu structure </pitch> I will probably continue to maintain and use it even if you decide to not include it into TDE.

Sorry and the late reply, it has been (still is) a super busy period and my time for TDE is still limited :-(

Again, I understand all too well :{

> Code wise, there will be several minor adjustment required at some point, but before that I would like to understand why you define 4 sort options in the enum but the menu only contains 3 options. There was no compelling reason to do that, since we only present 3 in the UI. I guess I thought to leave it in the code as a backward reference to how "Case Sensitive" sort is currently implemented, or as a placeholder in case we decide to somehow improve locale-aware sorting. > Build wide, I rebased the PR on top of current master and tried bulding in Debian bookworm. I ran into a FTBFS with the following linking error > ``` > [1/1] Linking CXX shared module konqueror/listview/konq_listview.so > /usr/bin/ld: konqueror/listview/CMakeFiles/konq_listview-module.dir/konq_listview.cpp.o: in function `KonqListView::slotSortOptionDialog()': > ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1039: undefined reference to `Sort_Options_Dialog::Sort_Options_Dialog(unsigned short, bool, bool)' > /usr/bin/ld: ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1094: undefined reference to `Sort_Options_Dialog::~Sort_Options_Dialog()' > /usr/bin/ld: ./obj-x86_64-linux-gnu/./konqueror/listview/konq_listview.cpp:1094: undefined reference to `Sort_Options_Dialog::~Sort_Options_Dialog()' > collect2: error: ld returned 1 exit status > ``` Just to rule out some stuff: 2 of the build-related files I changed were *libkonq/CMakeLists.txt* and *libkonq/Makefile.am,* so make sure those changes are propagated and make sure cmake is re-run. I use Gentoo portage for building on a "production support" system and here is the cmake command that is run before running make to build libkonq: ``` cmake -C /scratchfs/var_tmp_portage/portage/trinity-base/libkonq-9999/work/libkonq-9999_build/gentoo_common_config.cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_PREFIX=/usr/trinity/14 -DCMAKE_INSTALL_RPATH=/usr/trinity/14 -DWITH_ARTS=no -DBUILD_LIBKONQ=ON -DBUILD_ALL=OFF -DWITH_ALL_OPTIONS=OFF -DCMAKE_BUILD_TYPE=Gentoo -DCMAKE_TOOLCHAIN_FILE=/scratchfs/var_tmp_portage/portage/trinity-base/libkonq-9999/work/libkonq-9999_build/gentoo_toolchain.cmake -DWITH_GCC_VISIBILITY=OFF /scratchfs/var_tmp_portage/portage/trinity-base/libkonq-9999/work/libkonq-9999 ``` ... but that doesn't look like it provides much insight. > Last point is about the need or not for a sort dialog option. I know we debated about this on issue #252 and wanted to build this PR to see the result. At the same time, I looked at the Konqueror menu and perhaps it may be wiser to just use the current menu. I suspend my judgement till this PR is buildable and I can compare the two versions. Of course, I will also ask @SlavekB for his opinion at some point. To give you some history: at first I did not see any way of implementing a tri-value TDE menu option, so I decided to tackle this by learning how to create a QT dialog to do the job with radio buttons. And while I was at it, I thought I would put **all** sort-related option settings in this dialog. Later, I figured that this might depart a bit too much from the usual way of doing things via menu. I then looked harder and discovered there was a non-hacky way of implementing a 3-value menu option. So I did just that and made sure that the two interfaces coexisted properly. But I understand that this may result in one UI too many. I'm OK if you and @SlavekB decide not to include it. But I will say that **\<pitch\>** using my dialog is far faster and simpler than navigating a menu structure **\</pitch\>** I will probably continue to maintain and use it even if you decide to not include it into TDE. > Sorry and the late reply, it has been (still is) a super busy period and my time for TDE is still limited :-( Again, I understand all too well :{

backward reference to how "Case Sensitive" sort is currently implemented

Excuse me, I meant "Case Insensitive" sort. Like you, I will be glad to jettison this terminology. I hope you and @SlavekB like the menu / dialog labels I came up with.

I would like to understand why you define 4 sort options in the enum but the menu only contains 3 options.

There was no compelling reason to do that, since we only present 3 in the UI. I guess I thought to leave it in the code as a backward reference to how "Case Sensitive" sort is currently implemented, or as a placeholder in case we decide to somehow improve locale-aware sorting.

Ok thanks for the info. We could even remove it, but yeah no big deal to leave it there either.

Just to rule out some stuff: 2 of the build-related files I changed were
libkonq/CMakeLists.txt and libkonq/Makefile.am, so make sure those changes are propagated and make sure cmake is re-run.

Yes, the PR has been applied in full (after rebasing on top of master).

I use Gentoo portage for building on a "production support" system

Don't know much of the gentoo build system details, but the build in Debian was in a clean chroot build environment, without any previous version of tdebase installed.
Did your build happened in a system where a previously built version of the new tdebase was already available? If so it could be the difference.
Other than that, I build with cmake and ninja build, rather than make, but that should not make a difference either. I have not yet analyzed further the FTBFS, but surely it is related to this PR since there is no failure without it.

But I understand that this may result in one UI too many.

Let's discuss further when the PR is buildable.

Don't know much of the gentoo build system details, but the build in Debian was in a clean chroot build environment, without any previous version of tdebase installed.
Did your build happened in a system where a previously built version of the new tdebase was already available? If so it could be the difference.

OK, that's what is must be. There is no official set of Gentoo packages, so I am using the ones from https://scm.trinitydesktop.org/gitea/TDE/tde-packaging-gentoo.

That overlay (Gentoo terminology) takes the approach of splitting the upstream tdebase source package into multiple packages with inter-dependencies. The 2 packages I modified here are 'libkonq' and 'konqueror', directly corresponding to the respective subdirectories in the upstream source package. The idea is that someone could build TDE without 'konqueror' (as crazy as that sounds) but building 'konqueror' will always require 'libkonq' to be installed first.

Of course there should be a way to do this with a monolithic build of the entire tdebase source package -- I'm just not sure what needs to be changed to do that.

This all assumes that running nm -D /path-2-build/libkonq/libkonq.so | grep Sort_Options_Dialog displays on your test system shows a bunch of symbols. If it does not, then we have another problem.

I will try to troubleshoot this after I fix tdepim#54, which I want to close before R14.0.12 is out.

@VinceR
I managed to fix and build this PR, haven't done much testing yet. Now it's late night here, will post my comments about it tomorrow or during the first part of the week.

I rebased the branch on current master and pushed a fix that makes the PR buildable. The problem was with the symbol export of the sort dialog.
Also the .moc file is autogenerated during build, so it does not need to be included in the sources.

I will comment on the sort dialog tomorrow, after testing it.

Hi @VinceR,
I finally tested this PR. Here are some comments.

Menu entries:
although not technically 100% correct, I would suggest something like this:
"Character Code - Strict (...)" ==> "Character code case sensitive"
"Character Code - Modified (...)" ==> "Character code case insensitive"
"Natural order (locale-defined)" ==> "Natural order"
I think the proposed terms are more familiar with existing users and less likely to raise eyebrows.
What do you / @SlavekB think?

Sort dialog:

I see pros and cons in having it. The pro is to be able to set all the sorting options quickly, in one spot. The con is a fair bit of extra code for functionality which is already provided by the menu and that the user is likely to use very rarely (usually a user sets its sort options once and never touch them in most cases).
Personally I am more inclined to not including the sort dialog, but I will let @SlavekB make the final call on this.
The dialog needs some adjustment (see screenshot, I exagerated the size of the dialog on purpose):

the text is cut out at the right side
the buttons should be called "Ok" and "Cancel" as in most of the TDE dialogs and should be following the common TDE layout (you can see the "configure background" dialog as an example)
alternate and reverse sort options are available on the menu but not on the dialog

if we decide to keep the sort dialog, the menu entry should probably be called "Configure sort options" rather than "Sort option dialog", to be more inline with standard names used across TDE menus.

Once we agree what to do on the above, I will have some other comments about the code, but let's go step by step.

sort_dialog.png

122 KiB

Hi @VinceR,
I finally tested this PR. Here are some comments.

Menu entries:
although not technically 100% correct, I would suggest something like this:
"Character Code - Strict (...)" ==> "Character code case sensitive"
"Character Code - Modified (...)" ==> "Character code case insensitive"
"Natural order (locale-defined)" ==> "Natural order"
I think the proposed terms are more familiar with existing users and less likely to raise eyebrows.
What do you / @SlavekB think?

I would be OK with any type of labeling since I will know what it means but thought the desire was to get away from negative options like "case insensitive". OK, how about
"Character code (Unicode/ASCII)"
"Character code case insensitive"
"Order defined by locale (LC_COLLATE)"

Since there are a lot of nuances that may not be entirely clear to a user, perhaps a fuller explanation of sorting & collation options could be added to the Konqueror handbook

Sort dialog:

I see pros and cons in having it. The pro is to be able to set all the sorting options quickly, in one spot. The con is a fair bit of extra code for functionality which is already provided by the menu

I certainly understand and would have no problem if you and @SlavekB want to remove it. I would personally continue to keep it on my systems since I have gotten quite used to it (see next).

and that the user is likely to use very rarely (usually a user sets its sort options once and never touch them in most cases).

Of course, I cannot speak for other users but I will speak for myself: depending on what I am trying to do (file management, opening documents, etc.) and what particular directory I am looking at, I frequently switch the grouping and sort ordering options.

Personally I am more inclined to not including the sort dialog, but I will let @SlavekB make the final call on this.

Maybe the best way forward is to introduce the dialog as a separate issue / PR after this PR is settled.

The dialog needs some adjustment (see screenshot, I exagerated the size of the dialog on purpose):

the text is cut out at the right side

OK, now that's a good reason to put the dialog interface on hold. I just confirmed this on a laptop. It looks like DPI running into an unscalable QT3 dialog ... or my inexperience :)

the buttons should be called "Ok" and "Cancel" as in most of the TDE dialogs and should be following the common TDE layout (you can see the "configure background" dialog as an example)

alternate and reverse sort options are available on the menu but not on the dialog

Those are more like keyboard-driven actions that are listed in the menu mainly to document their existence. I sure hope users of those features are not using the menu -- it would be faster to just do mouse clicks on columns ... and even faster to use the keyboard shortcuts.

if we decide to keep the sort dialog, the menu entry should probably be called "Configure sort options" rather than "Sort option dialog", to be more inline with standard names used across TDE menus.

Once we agree what to do on the above, I will have some other comments about the code, but let's go step by step.

In view of the above, how about just ignoring the options dialog for the time being so we can discuss the other parts of the PR?

> Hi @VinceR, > I finally tested this PR. Here are some comments. > > Menu entries: > although not technically 100% correct, I would suggest something like this: > "Character Code - Strict (...)" ==> "Character code case sensitive" > "Character Code - Modified (...)" ==> "Character code case insensitive" > "Natural order (locale-defined)" ==> "Natural order" > I think the proposed terms are more familiar with existing users and less likely to raise eyebrows. > What do you / @SlavekB think? I would be OK with any type of labeling since I will know what it means but thought the desire was to get away from negative options like "case insensitive". OK, how about "Character code (Unicode/ASCII)" "Character code case insensitive" "Order defined by locale (LC_COLLATE)" Since there are a lot of nuances that may not be entirely clear to a user, perhaps a fuller explanation of sorting & collation options could be added to the Konqueror handbook > Sort dialog: > 1) I see pros and cons in having it. The pro is to be able to set all the sorting options quickly, in one spot. The con is a fair bit of extra code for functionality which is already provided by the menu I certainly understand and would have no problem if you and @SlavekB want to remove it. I would personally continue to keep it on my systems since I have gotten quite used to it (see next). > and that the user is likely to use very rarely (usually a user sets its sort options once and never touch them in most cases). Of course, I cannot speak for other users but I will speak for myself: depending on what I am trying to do (file management, opening documents, etc.) and what particular directory I am looking at, I frequently switch the grouping and sort ordering options. > Personally I am more inclined to not including the sort dialog, but I will let @SlavekB make the final call on this. Maybe the best way forward is to introduce the dialog as a separate issue / PR after this PR is settled. > 2) The dialog needs some adjustment (see screenshot, I exagerated the size of the dialog on purpose): > - the text is cut out at the right side OK, now that's a good reason to put the dialog interface on hold. I just confirmed this on a laptop. It looks like DPI running into an unscalable QT3 dialog ... or my inexperience :) > - the buttons should be called "Ok" and "Cancel" as in most of the TDE dialogs and should be following the common TDE layout (you can see the "configure background" dialog as an example) > - alternate and reverse sort options are available on the menu but not on the dialog > Those are more like keyboard-driven actions that are listed in the menu mainly to document their existence. I sure hope users of those features are not using the menu -- it would be faster to just do mouse clicks on columns ... and even faster to use the keyboard shortcuts. > 3) if we decide to keep the sort dialog, the menu entry should probably be called "Configure sort options" rather than "Sort option dialog", to be more inline with standard names used across TDE menus. > > Once we agree what to do on the above, I will have some other comments about the code, but let's go step by step. > In view of the above, how about just ignoring the options dialog for the time being so we can discuss the other parts of the PR?

Hi @VinceR,

"Character Code - Strict (...)" ==> "Character code case sensitive"
"Character Code - Modified (...)" ==> "Character code case insensitive"
"Natural order (locale-defined)" ==> "Natural order"

I would be OK with any type of labeling since I will know what it means but thought the desire was to get away from negative options like "case insensitive". OK, how about
"Character code (Unicode/ASCII)"
"Character code case insensitive"
"Order defined by locale (LC_COLLATE)"

Keep in mind most users won't have all the knowledge that you have on the matter (and that you so well explained on previous discussions). Easy and possibly familiar terms are easier for the general user to understand/relate to. So maybe a mix of the two proposal?
"Unicode based"
"Unicode based, case insensitive"
"Locale based"

Since there are a lot of nuances that may not be entirely clear to a user, perhaps a fuller explanation of sorting & collation options could be added to the Konqueror handbook

Yes, the handbook should definitely contain some explanation of the various order options. Suggest to keep it short in the handbook and eventually add (and refer to) a page on TDE's wiki where you can go in much more details about it (pretty much explain all the things you already covered here).

and that the user is likely to use very rarely (usually a user sets its sort options once and never touch them in most cases).

Of course, I cannot speak for other users but I will speak for myself: depending on what I am trying to do (file management, opening documents, etc.) and what particular directory I am looking at, I frequently switch the grouping and sort ordering options.

Fair enough, as I mentioned there are pros and cons in having the dialog. @SlavekB is busy with preparation of R14.0.12, so he is likely to comment on this PR only after that (== beginning of May afterward)

OK, now that's a good reason to put the dialog interface on hold. I just confirmed this on a laptop. It looks like DPI running into an unscalable QT3 dialog ... or my inexperience :)

the buttons should be called "Ok" and "Cancel" as in most of the TDE dialogs and should be following the common TDE layout (you can see the "configure background" dialog as an example)

Rule of thumb: TDE GUI is based on TQt, but most graphical elements are built on top of them. TQDialog is not used directly, instead KDialog is used across TDE.

alternate and reverse sort options are available on the menu but not on the dialog

Those are more like keyboard-driven actions that are listed in the menu mainly to document their existence. I sure hope users of those features are not using the menu

Agreed, they are (should be) mostly keyboard driven, but never assume all users will follow that principle. For completeness, those two options should be there, IMO.

In view of the above, how about just ignoring the options dialog for the time being so we can discuss the other parts of the PR?

That sounds like a good idea. How to proceed? Will you create a new branch/PR or rework the code here?

Hi @VinceR, > > "Character Code - Strict (...)" ==> "Character code case sensitive" > > "Character Code - Modified (...)" ==> "Character code case insensitive" > > "Natural order (locale-defined)" ==> "Natural order" > > I would be OK with any type of labeling since I will know what it means but thought the desire was to get away from negative options like "case insensitive". OK, how about > "Character code (Unicode/ASCII)" > "Character code case insensitive" > "Order defined by locale (LC_COLLATE)" Keep in mind most users won't have all the knowledge that you have on the matter (and that you so well explained on previous discussions). Easy and possibly familiar terms are easier for the general user to understand/relate to. So maybe a mix of the two proposal? "Unicode based" "Unicode based, case insensitive" "Locale based" > Since there are a lot of nuances that may not be entirely clear to a user, perhaps a fuller explanation of sorting & collation options could be added to the Konqueror handbook Yes, the handbook should definitely contain some explanation of the various order options. Suggest to keep it short in the handbook and eventually add (and refer to) a page on TDE's wiki where you can go in much more details about it (pretty much explain all the things you already covered here). > > and that the user is likely to use very rarely (usually a user sets its sort options once and never touch them in most cases). > > Of course, I cannot speak for other users but I will speak for myself: depending on what I am trying to do (file management, opening documents, etc.) and what particular directory I am looking at, I frequently switch the grouping and sort ordering options. Fair enough, as I mentioned there are pros and cons in having the dialog. @SlavekB is busy with preparation of R14.0.12, so he is likely to comment on this PR only after that (== beginning of May afterward) > OK, now that's a good reason to put the dialog interface on hold. I just confirmed this on a laptop. It looks like DPI running into an unscalable QT3 dialog ... or my inexperience :) > > > - the buttons should be called "Ok" and "Cancel" as in most of the TDE dialogs and should be following the common TDE layout (you can see the "configure background" dialog as an example) Rule of thumb: TDE GUI is based on TQt, but most graphical elements are built on top of them. TQDialog is not used directly, instead KDialog is used across TDE. > > - alternate and reverse sort options are available on the menu but not on the dialog > > Those are more like keyboard-driven actions that are listed in the menu mainly to document their existence. I sure hope users of those features are not using the menu Agreed, they are (should be) mostly keyboard driven, but never assume all users will follow that principle. For completeness, those two options should be there, IMO. > In view of the above, how about just ignoring the options dialog for the time being so we can discuss the other parts of the PR? That sounds like a good idea. How to proceed? Will you create a new branch/PR or rework the code here?

Hi @VinceR,

Keep in mind most users won't have all the knowledge that you have on the matter (and that you so well explained on previous discussions). Easy and possibly familiar terms are easier for the general user to understand/relate to. So maybe a mix of the two proposal?
"Unicode based"
"Unicode based, case insensitive"
"Locale based"

I think that's fine.

Since there are a lot of nuances that may not be entirely clear to a user, perhaps a fuller explanation of sorting & collation options could be added to the Konqueror handbook

Yes, the handbook should definitely contain some explanation of the various order options. Suggest to keep it short in the handbook and eventually add (and refer to) a page on TDE's wiki where you can go in much more details about it (pretty much explain all the things you already covered here).

A wiki article, maybe linked from handbook? I think that is called for and I will start composing some notes for that.

Fair enough, as I mentioned there are pros and cons in having the dialog. @SlavekB is busy with preparation of R14.0.12, so he is likely to comment on this PR only after that (== beginning of May afterward)

We will do the proposed dialog as a separate PR later.

Rule of thumb: TDE GUI is based on TQt, but most graphical elements are built on top of them. TQDialog is not used directly, instead KDialog is used across TDE.

MicheleC, I have been searching for some reasonable way to get educated in TDE application programming. I just tried playing around with programs in tdelibs/tdeui/tests but a lot of those are broken.

In view of the above, how about just ignoring the options dialog for the time being so we can discuss the other parts of the PR?

That sounds like a good idea. How to proceed? Will you create a new branch/PR or rework the code here?

I'll rework the code here. I will be away from a computer until next Friday, so things will have to wait until then.

Hi @VinceR,

"Unicode based"
"Unicode based, case insensitive"
"Locale based"

I think that's fine.

Great, let's go for it!

A wiki article, maybe linked from handbook? I think that is called for and I will start composing some notes for that.

Yes. Handbook contains basic explanation of the options, wiki page can contains details and long explanation of the problem and the solution.

MicheleC, I have been searching for some reasonable way to get educated in TDE application programming. I just tried playing around with programs in tdelibs/tdeui/tests but a lot of those are broken.

Will send you an email separately, to avoid polluting this PR with unrelated stuff.

I'll rework the code here.

Sounds good.

Hi @VinceR,
I noticed a new commit from a couple of days ago. Is the PR now ready for a new review?

Hi @VinceR,
I noticed a new commit from a couple of days ago. Is the PR now ready for a new review?

Yes, thanks.

@VinceR
I have rebased the branch on top of current master, so make sure you check it out if you will need to make any further changes.
I will now test it and then look at the code.

Seems to work nicely. What would be a way to easily test the difference betwen "Unicode based, case insensitive" and "Locale based", with locale being en.utf-8?

Seems to work nicely. What would be a way to easily test the difference betwen "Unicode based, case insensitive" and "Locale based", with locale being en.utf-8?

Try with these file names (listed in straight Unicode order)

1234
Bill.txt
Pates and other areas of the head.txt
Pet.txt
Pâté recipes.odt
[1234]
_Patrick.txt
_Vince.txt
p_a_t.txt
patrick.txt
{1234}

Besides bringing accented letter variants together, another noticeable difference with locale order is how non-alphanumeric ASCII characters affect sort order:

All other things being equal, non-alphanumeric characters sort before numeric characters which sort before alphabetical characters.
When non-alphanumeric characters are interspersed with alphanumeric characters, they are ignored for sorting purposes. This basically implements my originally proposed "Dictionary Order" setting.

While the "Unicode based, case insensitive" option may seem a bit pointless, I think that there is a case where one wants to purposely sort file names using special leading characters. For instance, I personally have directory names that start with '+' and '~' that I want to see listed respectively before and after other directory names that start with an ASCII letter. That "system" gets blown apart with locale order!

If after testing you feel pretty solid with the PR, I will try to implement this for icon view. Just because I don't care for that view doesn't mean I should ignore it :)

Thanks for the feedback @VinceR.
Looks nice (see screenshot - also @SlavekB ). Note: I did a small mistake when creating those folders automatically and I ended up with too many folders, but I had already taken the screenshot and didn't bother redoing it :-)
I also verified against another non-TDE app and the file listing in 'locale' mode matches perfectly with the other app.

While the "Unicode based, case insensitive" option may seem a bit pointless, I think that there is a case where

Indeed, this is a valid use case and I use it myself :-) As said in previous comments, different people have different preferences, so all 3 sort orders are valid.

I will try to implement this for icon view.

This may be debateble, since in icon view the user can place the icons wherever he wants, not even in a grid. But indeed it may be good to provide the options to them if they want to use it. AFAICT, currently 'locale' order is used in icon view mode.
In any case let's discuss this after merging this PR.

Another thing to implement after merging this PR will be to have the treeview ordering folders using the selected sort method. Currently it seems to use 'locale' sorting all the time. Again, let's do this after merging this PR.

An improvement could be to have keyboard shortcuts for the different sort modes, for example Alt+S + 1, Alt+S + 2, Alt+S + 3 to quickly switch between modes. This is especially useful for the use case mentioned earlier. What do you think? We could add this to this PR, IMO.

Next step: after hearing @SlavekB 's feedback, I will go through the code (this week), see if there is anything to adjust/comment about and then we can add this to R14.1.0!

Thanks for the contribution and patience. This is definitely a nice feature to have :-)

comparison-test.png

71 KiB

I'm a little confused. Within #252, it has been discussed that it will be good to provide all 4 sorting modes for users. But in the current PR, there are three. Did something change during work? The label Locale Order - &Improved is defined in the code, but such a mode does not seem to be provided to users.

I will try to implement this for icon view.

This may be debateble, since in icon view the user can place the icons wherever he wants, not even in a grid. But indeed it may be good to provide the options to them if they want to use it. AFAICT, currently 'locale' order is used in icon view mode.
In any case let's discuss this after merging this PR.

That's reasonable. Even if we don't outfit iconview with a UI, we probably should change the call in libkonq/tdefileivi.cpp: KFileIVI::compare() from view->m_pSettings->caseSensitiveCompare to new function stringCompare, since the former function uses what I am claiming to be invalid logic.

Another thing to implement after merging this PR will be to have the treeview ordering folders using the selected sort method. Currently it seems to use 'locale' sorting all the time. Again, let's do this after merging this PR.

I was not aware of this, need to look at treeview-specific code.

An improvement could be to have keyboard shortcuts for the different sort modes, for example Alt+S + 1, Alt+S + 2, Alt+S + 3 to quickly switch between modes. This is especially useful for the use case mentioned earlier. What do you think? We could add this to this PR, IMO.

Being a keyboard enthusiast, I think this is a GREAT idea! I must confess that I was not aware that you could make key assignments like you describe. How are those used? Do you press/release Alt+S, then press 1, 2, 3 ...?

I'm a little confused. Within #252, it has been discussed that it will be good to provide all 4 sorting modes for users. But in the current PR, there are three. Did something change during work? The label Locale Order - &Improved is defined in the code, but such a mode does not seem to be provided to users.

Yes, originally I thought that there was a use case for a sorting that "Locale-Aware" + "Case-Insensitive" (implemented by applying lower() function) but realized that this was a pointless combination since Locale-aware already accomplishes what "Case-Insensitive" is trying to provide: sorting the case variants of a letter together. See this comment and the comments preceding it in the original issue discussion for more background.

I probably should remove that 4th label in konq_sort_constants.h as it has caused too much confusion.

The definition of strings to be subject to translation is a problem in this way. These string are not pulled into the template and will not be provided to users for translation. There should be used I18N_NOOP(...).

Here is the intention for these strings to be available in applications that will use libkonq? Because it looks like they are currently used in the code either once (most LABEL_...) or not used at all (TOOLTIP_...). The question is whether to use I18N_NOOP, as mentioned above or whether to use the strings directly in the code, without defining in the header?

To clarify why I put these string constants in konq_sort_constants.h: As part of this PR, I had originally created a TQt dialog that provided UI access to all sort-related options. I did that before I figured out how to implement a tristate menu option. After I figured out how to implement the menu interface, I kept the dialog as an alternate user interface and figured that it and the menu should share the same UI strings defined in a common place. Subsequently, MicheleC and I agreed to defer the dialog UI to a separate PR.

Honestly, I am not sure how the translation process works. Consider the following 2 code snippets taken from konqueror/listview/konq_listview.cpp KonqListViewFactory::KonqListViewFactory()

Existing code:

m_paShowDot = new TDEToggleAction( i18n( "Show &Hidden Files" ), 0, this, TQT_SLOT( slotShowDot() ), actionCollection(), "show_dot" );

New code:

m_paOrder_UnicodeUnmodified = new TDEToggleAction( i18n(LABEL_UNICODE_UNMODIFIED), 0, this,  TQT_SLOT(slotOrder_UnicodeUnmodified()), actionCollection(), "order_unicode_unmodified");

I assumed, perhaps incorrectly, that i18n( "Show &Hidden Files" ) in existing code was being translated where a translation exists. Is the problem in the new code due to the fact that I am using a macro instead of literal text?

Honestly, I am not sure how the translation process works. Consider the following 2 code snippets taken from konqueror/listview/konq_listview.cpp KonqListViewFactory::KonqListViewFactory()

Existing code:
m_paShowDot = new TDEToggleAction( i18n( "Show &Hidden Files" ), 0, this, TQT_SLOT( slotShowDot() ), actionCollection(), "show_dot" );
New code:
m_paOrder_UnicodeUnmodified = new TDEToggleAction( i18n(LABEL_UNICODE_UNMODIFIED), 0, this,  TQT_SLOT(slotOrder_UnicodeUnmodified()), actionCollection(), "order_unicode_unmodified");
I assumed, perhaps incorrectly, that i18n( "Show &Hidden Files" ) in existing code was being translated where a translation exists. Is the problem in the new code due to the fact that I am using a macro instead of literal text?

In order for strings to become translated, there must be a way to extract them from source code. For TDE, therefore, xgettext is looking for i18n calls in the source code and extracts the strings found there. If there is a macro instead of a string, it is ignored. In order for the required string to be extracted, it is necessary to adjust the macro to use I18N_NOOP(...), which is otherwise empty macro, but is used for xgettext to identify other strings to be extracted.

#define LABEL_DIRECTORIES_FIRST       I18N_NOOP("&Directories displayed before non-directories")

This will ensure the string will be extracted into template, and information about the origin of the translated string will refer to the header file.

In order for strings to become translated, there must be a way to extract them from source code. For TDE, therefore, xgettext is looking for i18n calls in the source code and extracts the strings found there. If there is a macro instead of a string, it is ignored. In order for the required string to be extracted, it is necessary to adjust the macro to use I18N_NOOP(...), which is otherwise empty macro, but is used for xgettext to identify other strings to be extracted.
#define LABEL_DIRECTORIES_FIRST       I18N_NOOP("&Directories displayed before non-directories")
This will ensure the string will be extracted into template, and information about the origin of the translated string will refer to the header file.

OK, I understand now. I will make the necessary corrections and post an update.

Being a keyboard enthusiast, I think this is a GREAT idea! I must confess that I was not aware that you could make key assignments like you describe. How are those used? Do you press/release Alt+S, then press 1, 2, 3 ...?

You press Alt+S and then a number. I tested it out here and it works fine. You need to tick the "multi key" in the dialog where you select the shortcut keys.

This is the first part of code review, I have not gone through all yet.
There are several things to adjust, so let's do this first, then I will take a further look.
Two general notes:

try to follow conventions used around in the same files (naming conventions, function styles, ...)
there are a lot of unnecessary comments in the code. Although they may be good for you when you are coding and testing, they should be removed from the final code. The reason being:
a. make the code longer to read
b. statistically over time comments tend to disalign with the code
A good practice (from experience devs, not me) is that comment should be added only when the code cannot clearly explain a specific thing. Comments line ".h file is important" or "We define the following UI text items once to be used in different places" are not very useful. Comments like those in "libkonq/konq_string_compare.h" are instead meaningful because they explain the difference between the options.

MicheleC,

That's a lot of reviewing :)

I will be travelling through next week and away from my main development system. I will try to take a look at this while I am gone but won't be able to make changes until I return.

I am not sure I agree with your comment about comments. I personally would have preferred a LOT MORE comments in the code to help me understand the reasons for why code was done a certain way. That said, I will maintain this information in personal notes.

It is certainly true that there is a lot of debugging stuff I have in the code that needs removing.

VinceR

That's a lot of reviewing :)

That was about half the code. Still have to comment on the rest :-)

I will be travelling through next week

Thanks for the info. Not a problem, take you time.

I am not sure I agree with your comment about comments.

That is definitely a subjective point :-)
If you have some time to pass, you may want to take a look at this video on comments from Bob Martin, if may give you a broader perspective :-)

@VinceR
once you are done with the first round of fixup, please push the updated code on this PR and let me know, so I can review the remaining part :-)

Anticipating questions about new sort comparison routine stringCompare declared and defined in tdebase/libkonq/konqstringcompare.h:

I wanted to place the code (and associated constants) in a central area where it would be available not only to Konqueror listview (and iconview in future), but also to other applications that might need to have a uniform way of conducting sort string comparison. However it could be argued that tdebase/libkonq is not "central" enough:
- Maybe tdelibs/tdecore would be a more application-neutral place?
- Since the routine is basically a TQString comparison function, maybe tqt3/src/tools is the best place. However, if someday TDE is ported to QT5, that might present a problem.
Since the code is small and is likely to be used in sort comparison functions (which get called a lot during a sort), I wanted to implement it as an inline function to avoid extra function call overhead. However:
- I don't really know how important this concern is on modern systems.
- Part of code being replaced in KonqBaseListViewItem::compare already makes an "extra" function call to KonqFMSettings::caseSensitiveCompare.

What do you think?

Looks pretty good, just a few comments to adjust.
I will do a functionality test, just to confirm it still works fine.

Hi @VinceR,

I wanted to place the code (and associated constants) in a central area where it would be available not only to Konqueror listview (and iconview in future), but also to other applications that might need to have a uniform way of conducting sort string comparison. However it could be argued that tdebase/libkonq is not "central" enough:

Maybe tdelibs/tdecore would be a more application-neutral place?

Since the routine is basically a TQString comparison function, maybe tqt3/src/tools is the best place. However, if someday TDE is ported to QT5, that might present a problem.

This is a good idea. tqt3 would be the best place, as you suggested it is just comparing strings, so there is nothing TDE-specific that would require moving it to tdelibs/tdebase.
We can either merge the code as is first, and do the move to tqt3 later.
Or you can choose to first expand tqt3 with string comparison (both TQString and TQCString) and then rework this PR. The choice is yours :-)

PS: we won't migrate to TQt5... most likely we will backport and expand tqt3 after R14.1.0 is out.

Since the code is small and is likely to be used in sort comparison functions (which get called a lot during a sort), I wanted to implement it as an inline function to avoid extra function call overhead. However:

I don't really know how important this concern is on modern systems.

Part of code being replaced in KonqBaseListViewItem::compare already makes an "extra" function call to KonqFMSettings::caseSensitiveCompare.

inline is only a suggestion to the compiler, but there is no guarantee the function will be inlined or not. We can leave the inline there, it won't hurt. If the compiler thinks it is too big, it won't inline it in the end.

We can either merge the code as is first, and do the move to tqt3 later.
Or you can choose to first expand tqt3 with string comparison (both TQString and TQCString) and then rework this PR. The choice is yours :-)

Put that way, I say let's stay the course :)

most likely we will backport and expand tqt3

The one thing that would really benefit tqt3 would be to implement some type of support for High DPI. Scaling text is easy, getting widgets and icons to automatically scale with the text would be great.

after R14.1.0 is out

Do you folks have an approximate time frame for that?

Put that way, I say let's stay the course :)

I agree with you, let's take one step at a time.

The one thing that would really benefit tqt3 would be to implement some type of support for High DPI. Scaling text is easy, getting widgets and icons to automatically scale with the text would be great.

Widget already scales if the font is changed.
Font DPI can already be changed between 64 and 512 for sessions (not yet for tdm - not from GUI at least).
The thing really needed here is bigger icons :-)
On a side note: Qt4 has some nice features, so backporting some of its code is something I would like to work on at some point

after R14.1.0 is out
Do you folks have an approximate time frame for that?

This year, it has been delayed way too much already :-(

I did a build and test, looks good to merge once the comments above are addressed.
I also like your choice of shortcuts (Alt+1/2/3) instead of the multikey sequence I initially suggested.

The one thing that would really benefit tqt3 would be to implement some type of support for High DPI. Scaling text is easy, getting widgets and icons to automatically scale with the text would be great.

Widget already scales if the font is changed.
Font DPI can already be changed between 64 and 512 for sessions (not yet for tdm - not from GUI at least).

I need to revisit this. I posted a lament, stated years ago here: https://www.spinics.net/lists/trinity-devel/msg00332.html. I did some Rnd and played around with some solutions, including a variant on the Phase theme but I didn't go any further. The problem is that all of the good high DPI laptops go to my kids. I've got the big high resolution monitor but it ends up at only 100 DPI :)

The thing really needed here is bigger icons :-)

I think the solution will ultimately require SVG icons.

This year, it has been delayed way too much already :-(

OK, I've got a couple of more PRs to finish up before the feature freeze :)

I posted a lament, stated years ago here

I think your points on widgets without text (scrollbars, arrows, ...) are probably still true and likely show up to small on HiDPI screens. We may need further work on that.

I think the solution will ultimately require SVG icons.

Possibly. More importantly we need someone will to work on those icons :-)

OK, I've got a couple of more PRs to finish up before the feature freeze :)

Happy to review/discuss any improvement you wish to put forward. The more people contribute, the better TDE can get :-)

Will send you an email separately, to avoid polluting this PR with unrelated stuff.

Btw, did you ever get my email? I have not seen any feedback, so wondering if you got that in first place...

It looks good. Ready to be squashed and merged into master.
Will wait for a final approval/comments from @SlavekB before proceeding with that.

Will send you an email separately, to avoid polluting this PR with unrelated stuff.

Btw, did you ever get my email? I have not seen any feedback, so wondering if you got that in first place...

I did and I apologize for not responding. The topic of TDE (vs. TQT) application/dialog development is still a very pertinent one. Would the mailing list be an appropriate place to continue that discussion?

I did and I apologize for not responding.

No worries, just making sure it reached you.

The topic of TDE (vs. TQT) application/dialog development is still a very pertinent one. Would the mailing list be an appropriate place to continue that discussion?

Mailing list is fine. Or if you are interested you can also join our developer's chat room on jabbim, Slavek and I are usually there when we are online

Note for @SlavekB: once this PR is ok to ok, I will squash, rebase and merge.

I have updated the PR as we discussed before, squashed and uploaded to the the same branch. @VinceR is credited as the author of all the work.

@SlavekB, IMO this PR is ready to be merged and included into R14.1.0. Waiting for your comments/go ahead before doing that.

Everything looks good. It seems that we can merge it.
Thank you all for your efforts and a good move forward.

@VinceR
finally this has been merged into R14.1.0-dev branch!
Thanks for the good work and the patience throughout the process. Looking forward for the next PR :-)

Note: Because there were added installation of two new headers, I assume that we need to adjust libkonq4-tinity-dev.install in TDE/tde-packaging to add konq_sort_constants.h and konq_string_compare.h to the package.

Note: Because there were added installation of two new headers, I assume that we need to adjust libkonq4-tinity-dev.install in TDE/tde-packaging to add konq_sort_constants.h and konq_string_compare.h to the package.

ah yes, thanks for the reminder. In fact there is already a branch in tde-packaging for it. I will update and merge soon.

EDIT: there isn't, it was local to my repo only :-(

tde packaging repo updated

Just getting back to work now :)

Thanks to all for helping bring this PR to closure.

I'll keep Issue 252 open and add some notes about what remaining things need to be done in order to to fully resolve it.

   <Separator/>
   <Action name="show_dot"/>
   <Menu name="sort"><text>Sort</text>
   <Menu name="sort"><text>&amp;Sort</text>

   <Action name="show_dot"/>
   <Menu name="sort"><text>Sort</text>
   <Menu name="sort"><text>&amp;Sort</text>
     <!--TBD <TearOffHandle />-->

     <!--TBD <TearOffHandle />-->
     <Action name="order_unicode_unmodified"/>
     <Action name="order_unicode_case_insensitive"/>
     <!--<Action name="sort_caseinsensitive"/>-->

   <Action name="show_dot"/>
   <Menu name="sort"><text>Sort</text>
   <Menu name="sort"><text>&amp;Sort</text>
     <!--TBD <TearOffHandle />-->

     i18n(LABEL_LOCALE_UNMODIFIED), 0, this,
     TQT_SLOT(slotOrder_Locale()),
     actionCollection(), "order_locale_defined"
   );

 #define LABEL_LOCALE_MODIFIED         "Locale Order - &Improved"
 #define LABEL_DIRECTORIES_FIRST       "&Directories displayed before non-directories"
 #define LABEL_HIDDEN_FIRST            "&Hidden displayed before non-hidden"

 #include <unistd.h>
 #include <kinstance.h>
 #include <konq_sort_constants.h> // Important

 {
   TextSortOrder sortOrder = UNICODE_UNMODIFIED ;
   kdWarning() << "Setting name order = " << sortOrder << endl;
 //kdDebug(1202) << "Setting name order = " << sortOrder << endl;

 #include <konq_propsview.h>
 #include "konq_listviewwidget.h"
 #include <konq_sort_constants.h> // Important

   void slotSaveColumnWidths();  // delayed
   void slotHeaderClicked(int sec);
   void slotOrder_UnicodeUnmodified();

   TDEAction *m_paUnselectAll;
   TDEAction *m_paInvertSelection;
   TDEAction *m_paSortOptionDialog;

   TDEToggleAction *m_paShowPermissions;
   TDEToggleAction *m_paShowURL;
   TDEToggleAction *m_paOrder_UnicodeUnmodified;

 #include <kurl.h>
 #include <libkonq_export.h>
 #include "konq_sort_constants.h" // Important


				`#define TOOLTIP_DIRECTORIES_FIRST I18N_NOOP("Group directories before non-directories")`
				`#define TOOLTIP_HIDDEN_FIRST I18N_NOOP("Group hidden before non-hidden")`

       return a.lower().localeAwareCompare( b.lower() );
       break;
     */

    return pm;
 }
 #include "konq_string_compare.h" // defines stringCompare(), called in the next function

       If we get this point, we are comparing text columns (e.g file name). We will
       call the previously defined general purpose inline function stringCompare()
       using current value of m_pListViewWidget->m_sortOrder.
    */

				`Please leave this comment in this header until above changes have been made.`

				`*/`

   m_paOrderUnicodeCaseInsensitive->setToolTip( i18n( "Like above but with lower/upper case ASCII letters adjacent" ) );
   m_paOrderUnicodeCaseInsensitive->setChecked(initialOrder == UNICODE_CASEINSENSITIVE);
   m_paOrderLocale = new TDEToggleAction( i18n( "&Locale Order" ), ALT+Key_3, this,

Proposed resolution for issue # 252 for Konqueror listviews #253

Reviewers