WIP: Implement TDE/tdebase#270 for tdelibs: Extend meaning of "Hidden" files. #163

This pull request addresses the tdelibs portion - there will also be a separate and related pull request for tdebase.

The changes are based on proposed patch submitted in response to a KDE bug filed many years ago.

It should be noted that most of the changes are concerned with changing *DotFiles* to *HiddenFiles* in names. This look like the riskiest part.

The changes have been successfully tested in a test environment -- so far, so good.

TO-DO:

Move testing to a "real" (daily use) environment.
Complete & test related changes to tdebase/libkonq & tdebase/konqueror.
Create a user interface for editing associated configuration.

This is a note to myself, no need for comments / review unless you want.

Test Case # 1: demonstrate use of multiple hidden file specifications on normal size directories.

Test Setup

Konqueror view mode: Detailed List View
HiddenFileSpec: *~,TEMP*,temp* (note that these are globs, not a regex's)
Location: a test user's home directory, in a chroot.

Results

Works correctly, with no apparent issues. Specifics using a more realistic home directory (mine) will be posted once I start doing some "no better test than production" testing:)

This is a note to myself, no need for comments / review unless you want.

Test Case # 2: demonstrate use of a single hidden file specification on large directory.

Test Setup

Konqueror view mode: Detailed List View
HiddenFileSpec: lib*
Location: /usr/lib64 (7160 hidden items + 320 non-hidden items), in a chroot.

Test Scenarios

Scenario 1a: "Show Hidden Files" checked, konqueror is run for first time. Result: works correctly without noticeable delay.
Scenario 1b: While konqueror is running, change "Show Hidden Files" from checked to unchecked. Result: works correctly without noticeable delay.
Scenario 2a: "Show Hidden Files" unchecked, konqueror is run for first time. Result: works correctly without noticeable delay.
Scenario 2b: While konqueror is running, change "Show Hidden Files" from unchecked to checked. Result: works correctly with significant (~20 second) delay.

Speculation

I do not think the delay in Scenario 2b is caused by any of the new code. In other words, I think that any directory with 7160 dotfiles + 320 non-dotfiles would manifest this problem currently.

Here is an excerpt from the code in tdelibs/tdeio/kdirlister.cpp, with my unsubstantiated comments:

if ( d->changes & HIDDEN_FILES )
{
  if ( d->isShowingHiddenFiles ) {
    addNewItem( *kit );
  }
  else {
    emit deleteItem( *kit );
   // This looks like it could be REALLY SLOW for large KDirLister structures
  }

I speculate that it would be more efficient to just initialize another KDirLister structure rather than trying all of these deletes on such a large list.

I do not think the delay in Scenario 2b is caused by any of the new code. In other words, I think that any directory with 7160 dotfiles + 320 non-dotfiles would manifest this problem currently.

I have confirmed this. This is limitation (or bug) in current KDirLister::emitChanges() and is not related to the PR.

While testing the commit from a couple of days ago, I ran into a problem wherein the Konqueror kfind plugin was blowing up on Close, whether or not any type of search had been activated. I thought the problem was related to the missing delete(matcher) (corrected in the latest commit) but that was not it.

What I discovered was that when one makes a change to a well-known object such as KDirLister, one should probably recompile everything that depends on it. My Gentoo system splits up the tdebase into multiple "applications", so I couldn't simply recompile a package called tdebase.

For me, the solution to this particular problem was rebuilding (in Gentoo speak) the trinity-base/kfind package which is part of tdebase. Other tdebase packages did not seem affected.

What I discovered was that when one makes a change to a well-known object such as KDirLister, one should probably recompile everything that depends on it.

There shouldn't be a need to recompile, unless you are changing static libraries. But it is probably recommended to logout and login again, to make sure the new libraries are used. I have seen similar issues when rebuilding "core" elements and a logout/login is a clean start.

What I discovered was that when one makes a change to a well-known object such as KDirLister, one should probably recompile everything that depends on it.

There shouldn't be a need to recompile, unless you are changing static libraries. But it is probably recommended to logout and login again, to make sure the new libraries are used. I have seen similar issues when rebuilding "core" elements and a logout/login is a clean start.

It is always essential whether the patch represents a change in the public API/ABI or only internal. It should be kept in mind that the inline function defined in the header may manifest itself as a hidden API/ABI change. If the API/ABI is unchanged, then it should not require rebuild and should be sufficient logout/login, as mentioned by @MicheleC, or run /opt/trinity/bin/tdeinit.

@VinceR
I will start reviewing and adding comments from tomorrow. It may take a few days to complete, due to my workload and some days off with family.
Again, apologies for the delay, July has been way too busy here.

The problem I see with the current use of matcher in KFileItem is that each time you want to see if a file is hidden (KFileItem::isHidden()), we first call KFileItem::SetHidden() to set the matcher and the property, which is not very clean.
I see two possible ways to improve this, depending on what we want to achieve.

pass the matcher to KFileItem::isHidden() and determine if a file is hidden or not at that moment. This has the benefit to require less code changes (the whole m_bHiddenFile logic is not needed anymore), but has the drawback to require evaluation at each call of KFileItem::isHidden(), which possibly results in worse performances.
The idea behind this solution is that KFileItem could be visible or hidden depending on the matcher in use, and we don't need to modify the KFileItem at all, regardless of the matcher in use
if we don't want the overhead each time we call KFileItem::isHidden(), we can decide that a KFileItem is visible/hidden at the time it is created or when a matcher is modified. This requires modifying all KFileItems each time we create them or when we change the matcher (this also implies remembering all the KFileItem that we need to modify when we change a the matcher). It also means that we can't use different matchers on the same KFileItem object.

IMO, option 1 is a better design and pretty much what the original code was doing, although with a fixed rule to decide whether a file was visible or not. We would need to see if performances would be drastically affected or not, but I think it should not be much worse than before.

@VinceR, @SlavekB what is your opinion?

The problem I see with the current use of matcher in KFileItem is that each time you want to see if a file is hidden (KFileItem::isHidden()), we first call KFileItem::SetHidden() to set the matcher and the property, which is not very clean.
I see two possible ways to improve this, depending on what we want to achieve.

pass the matcher to KFileItem::isHidden() and determine if a file is hidden or not at that moment. This has the benefit to require less code changes (the whole m_bHiddenFile logic is not needed anymore), but has the drawback to require evaluation at each call of KFileItem::isHidden(), which possibly results in worse performances.
The idea behind this solution is that KFileItem could be visible or hidden depending on the matcher in use, and we don't need to modify the KFileItem at all, regardless of the matcher in use

@VinceR, @SlavekB what is your opinion?

An earlier implementation did something like option 1. I now realize that there is probably no need for separate isHidden() and SetHidden() functions. BUT ... the problem I struggled with was the fact that isHidden() is being called from other places than within KDirLister code. I do think we still need m_bHiddenFile to remember a file's "hideability" for such function calls.

Would the following code address your concern? We would get rid of SetHidden() and call isHidden(matcher) within the KDirLister code

bool KFileItem::isHidden(TDEIO::HiddenFileMatcher *matcher = nullptr) const
{
  if ( matcher != nullptr ) {
    // Re-evaluate file's "hideability" using the passed matcher and record
    // that for future calls to this function where no matcher is passed
    if ( !m_url.isEmpty() )
      m_bHiddenFile = matcher->match( m_url.fileName() );
    else // should never happen
      m_bHiddenFile = matcher->match( m_strName );
  }

  if ( m_hidden != Auto )
      return m_hidden == Hidden;
  return m_bHiddenFile;

  /* FIXME: Who else calls this function? We see calls in these functions

     - tdeio/tdeio/kdirlister.cpp/KDirLister::emitChanges()
     - tdeio/tdeio/kdirlister.cpp/KDirLister::matchesFilter()
     
     but testing reveals that it is also being called elsewhere.
  */
}

> The problem I see with the current use of `matcher` in `KFileItem` is that each time you want to see if a file is hidden (`KFileItem::isHidden()`), we first call `KFileItem::SetHidden()` to set the matcher and the property, which is not very clean. > I see two possible ways to improve this, depending on what we want to achieve. > > 1) pass the matcher to `KFileItem::isHidden()` and determine if a file is hidden or not at that moment. This has the benefit to require less code changes (the whole `m_bHiddenFile` logic is not needed anymore), but has the drawback to require evaluation at each call of `KFileItem::isHidden()`, which possibly results in worse performances. > The idea behind this solution is that `KFileItem` could be visible or hidden depending on the matcher in use, and we don't need to modify the KFileItem at all, regardless of the matcher in use > > @VinceR, @SlavekB what is your opinion? An earlier implementation did something like option 1. I now realize that there is probably no need for separate `isHidden()` and `SetHidden()` functions. BUT ... the problem I struggled with was the fact that `isHidden()` is being called from other places than within KDirLister code. I do think we still need `m_bHiddenFile` to remember a file's "hideability" for such function calls. Would the following code address your concern? We would get rid of `SetHidden()` and call `isHidden(matcher)` within the KDirLister code ``` bool KFileItem::isHidden(TDEIO::HiddenFileMatcher *matcher = nullptr) const { if ( matcher != nullptr ) { // Re-evaluate file's "hideability" using the passed matcher and record // that for future calls to this function where no matcher is passed if ( !m_url.isEmpty() ) m_bHiddenFile = matcher->match( m_url.fileName() ); else // should never happen m_bHiddenFile = matcher->match( m_strName ); } if ( m_hidden != Auto ) return m_hidden == Hidden; return m_bHiddenFile; /* FIXME: Who else calls this function? We see calls in these functions - tdeio/tdeio/kdirlister.cpp/KDirLister::emitChanges() - tdeio/tdeio/kdirlister.cpp/KDirLister::matchesFilter() but testing reveals that it is also being called elsewhere. */ } ```

An earlier implementation did something like option 1. I now realize that there is probably no need for separate isHidden() and SetHidden() functions. BUT ... the problem I struggled with was the fact that isHidden() is being called from other places than within KDirLister code. I do think we still need m_bHiddenFile to remember a file's "hideability" for such function calls.

Hi @VinceR, sorry for the late reply, I was on a trip with the family for a few days.
I thought more about this and I am even more convinced that we should implement option 1. The first implementation in this PR does not perform any faster/better since setHidden is called whenever we want to call isHidden, so there would be no performance loss at all.

Would the following code address your concern? We would get rid of SetHidden() and call isHidden(matcher) within the KDirLister code

bool KFileItem::isHidden(TDEIO::HiddenFileMatcher *matcher = nullptr) const
{
  if ( matcher != nullptr ) {
    // Re-evaluate file's "hideability" using the passed matcher and record
    // that for future calls to this function where no matcher is passed
    if ( !m_url.isEmpty() )
      m_bHiddenFile = matcher->match( m_url.fileName() );
    else // should never happen
      m_bHiddenFile = matcher->match( m_strName );
  }

  if ( m_hidden != Auto )
      return m_hidden == Hidden;
  return m_bHiddenFile;

  /* FIXME: Who else calls this function? We see calls in these functions

     - tdeio/tdeio/kdirlister.cpp/KDirLister::emitChanges()
     - tdeio/tdeio/kdirlister.cpp/KDirLister::matchesFilter()
     
     but testing reveals that it is also being called elsewhere.
  */
}

Pretty good, just a couple of improvements.

I don't think we need m_bHiddenFile at all, we can simply evaluate on the spot whether a file is hidden.
We should abide by any choice that was made with m_hidden first.
the FIXME comment is not necessary and likely to become outdated as soon as the call is used in any other part of TDE. If it was jsut to discuss a topic in this PR, we can do a search through the code and discuss it here :-)
I am doubtful whether to use a default value for matcher because it would provide a "false" result. I think better to provide a matcher whenever we want to use this functionality. What do you think? If we keep the default nullptr value, we should probably provide the original logic based on the filename as a result for the time when no matcher is provided.

Basically a logic similar to the original code, but where the part related to the hidden test is replaced with the call to the matcher.
If we use m_bHiddenFile, we would have to

bool KFileItem::isHidden(TDEIO::HiddenFileMatcher *matcher) const
{
  if ( m_hidden != Auto )
      return m_hidden == Hidden;
  
  // Evaluate file's "hideability" using the passed matcher
  if ( !m_url.isEmpty() )
    return matcher->match( m_url.fileName() );
  else // should never happen
    return matcher->match( m_strName );
}

In the next 2 or 3 days I should have enough time to complete the review of this PR. Will likely add some more comments.

> An earlier implementation did something like option 1. I now realize that there is probably no need for separate `isHidden()` and `SetHidden()` functions. BUT ... the problem I struggled with was the fact that `isHidden()` is being called from other places than within KDirLister code. I do think we still need `m_bHiddenFile` to remember a file's "hideability" for such function calls. Hi @VinceR, sorry for the late reply, I was on a trip with the family for a few days. I thought more about this and I am even more convinced that we should implement option 1. The first implementation in this PR does not perform any faster/better since `setHidden` is called whenever we want to call `isHidden`, so there would be no performance loss at all. > Would the following code address your concern? We would get rid of `SetHidden()` and call `isHidden(matcher)` within the KDirLister code > > ``` > bool KFileItem::isHidden(TDEIO::HiddenFileMatcher *matcher = nullptr) const > { > if ( matcher != nullptr ) { > // Re-evaluate file's "hideability" using the passed matcher and record > // that for future calls to this function where no matcher is passed > if ( !m_url.isEmpty() ) > m_bHiddenFile = matcher->match( m_url.fileName() ); > else // should never happen > m_bHiddenFile = matcher->match( m_strName ); > } > > if ( m_hidden != Auto ) > return m_hidden == Hidden; > return m_bHiddenFile; > > /* FIXME: Who else calls this function? We see calls in these functions > > - tdeio/tdeio/kdirlister.cpp/KDirLister::emitChanges() > - tdeio/tdeio/kdirlister.cpp/KDirLister::matchesFilter() > > but testing reveals that it is also being called elsewhere. > */ > } > Pretty good, just a couple of improvements. 1) I don't think we need `m_bHiddenFile` at all, we can simply evaluate on the spot whether a file is hidden. 2) We should abide by any choice that was made with m_hidden first. 3) the FIXME comment is not necessary and likely to become outdated as soon as the call is used in any other part of TDE. If it was jsut to discuss a topic in this PR, we can do a search through the code and discuss it here :-) 4) I am doubtful whether to use a default value for `matcher` because it would provide a "false" result. I think better to provide a matcher whenever we want to use this functionality. What do you think? If we keep the default `nullptr` value, we should probably provide the original logic based on the filename as a result for the time when no matcher is provided. Basically a logic similar to the original code, but where the part related to the hidden test is replaced with the call to the matcher. If we use `m_bHiddenFile`, we would have to ``` bool KFileItem::isHidden(TDEIO::HiddenFileMatcher *matcher) const { if ( m_hidden != Auto ) return m_hidden == Hidden; // Evaluate file's "hideability" using the passed matcher if ( !m_url.isEmpty() ) return matcher->match( m_url.fileName() ); else // should never happen return matcher->match( m_strName ); } ``` In the next 2 or 3 days I should have enough time to complete the review of this PR. Will likely add some more comments.

To clarify why I think there is a need for property KFileItem::m_bHiddenFile:

As you can see, my KFileItem::isHidden(matcher) code uses this property to cache the most recent result of matcher->match(...). Why do that? Because there are other callers of this function that do NOT pass a TDEIO::HiddenFileMatcher object to it. Property m_bHiddenFile is for their benefit.

Options for handling these callers are as follows:

Modify the callers to pass, directly or indirectly, an associated KDirLister::match object to KFileItem::isHidden(). That won't work in code that has no relationship to or visibility into an associated KDirLister.
Handle those callers by returning the result of the most recently cached call to the matcher. If the KFileItem object was not created by a KDirLister object, then return false . This is what I am proposing to do.
Return FALSE meaning no file will be hidden. I think that would be preferable to the next option.
Return a value based on the legacy dotfile test.

Here are the calls to KFileItem::isHidden() in tdelibs and tdebase that I was able to detect:

tdelibs/tdeio/tdeio/tdefileitem.cpp/KFileItem::overlays() which called by:
  tdelibs/tdeio/tdefile/tdefileiconview.cpp/KFileIconView::gotPreview(...)
  tdebase/libkonq/konq_iconviewwidget.cpp/KonqIconViewWidget::slotPreview(...)

The code appears to only be concerned with applying transparency to icons for hidden files. There is undoubtedly a KDirLister involved somewhere but I didn't see any obvious way to get access to it. But there would be no need to do any code modifications with my preferred option 2 -- the meaning of "hidden" will always be current. Or we could go for option 3 and abandon the icon transparency feature. Not being a big IconView user, I could live with that :)

tdelibs/tdeio/tdeio/tdefilefilter.cpp/KSimpleFileFilter::passesFilter(...)

I could not find any users of the KSimpleFileFilter class. Maybe that code could be amended to add and implement its own TDEIO::HiddenFileMatcher *matcher property as was done in with KDirLister. We could do that.

tdebase/tdeioslave/trash/testtrash.cpp/TestTrash::stat*()
# 5 functions, executing this:
  assert( !item.isHidden() );

Test trash program that doesn't get compiled, hmm. Anyway, with no KDirLister, the assertion should always succeed.

Of course, there might be calls to KFileItem::isHidden() in other parts of the TDE codebase.

To clarify why I think there is a need for property `KFileItem::m_bHiddenFile`: As you can see, my `KFileItem::isHidden(matcher)` code uses this property to cache the most recent result of `matcher->match(...)`. Why do that? Because there are other callers of this function that do **NOT** pass a `TDEIO::HiddenFileMatcher` object to it. Property `m_bHiddenFile` is for their benefit. Options for handling these callers are as follows: 1. Modify the callers to pass, directly or indirectly, an associated `KDirLister::match` object to `KFileItem::isHidden()`. That won't work in code that has no relationship to or visibility into an associated `KDirLister`. 1. Handle those callers by returning the result of the most recently cached call to the matcher. If the `KFileItem` object was **not** created by a `KDirLister` object, then return false . *This is what I am proposing to do*. 1. Return FALSE meaning no file will be hidden. I think that would be preferable to the next option. 1. Return a value based on the legacy dotfile test. ----- Here are the calls to `KFileItem::isHidden()` in tdelibs and tdebase that I was able to detect: ``` tdelibs/tdeio/tdeio/tdefileitem.cpp/KFileItem::overlays() which called by: tdelibs/tdeio/tdefile/tdefileiconview.cpp/KFileIconView::gotPreview(...) tdebase/libkonq/konq_iconviewwidget.cpp/KonqIconViewWidget::slotPreview(...) ``` The code appears to only be concerned with applying transparency to icons for hidden files. There is undoubtedly a `KDirLister` involved somewhere but I didn't see any obvious way to get access to it. But there would be no need to do any code modifications with my preferred option 2 -- the meaning of "hidden" will always be current. Or we could go for option 3 and abandon the icon transparency feature. Not being a big IconView user, I could live with that :) ``` tdelibs/tdeio/tdeio/tdefilefilter.cpp/KSimpleFileFilter::passesFilter(...) ``` I could not find any users of the `KSimpleFileFilter` class. Maybe that code could be amended to add and implement its own `TDEIO::HiddenFileMatcher *matcher` property as was done in with `KDirLister`. We could do that. ``` tdebase/tdeioslave/trash/testtrash.cpp/TestTrash::stat*() # 5 functions, executing this: assert( !item.isHidden() ); ``` Test trash program that doesn't get compiled, hmm. Anyway, with no `KDirLister`, the assertion should always succeed. ----- Of course, there might be calls to `KFileItem::isHidden()` in other parts of the TDE codebase.

Hi @VinceR

As you can see, my KFileItem::isHidden(matcher) code uses this property to cache the most recent result of matcher->match(...). Why do that? Because there are other callers of this function that do NOT pass a TDEIO::HiddenFileMatcher object to it. Property m_bHiddenFile is for their benefit.

It is a very valid point and it raises two important questions.

How should we handle things from a global perspective?
How to set a global matcher?

The more I think about the functionality we are trying to implement, the more I see a need to have the following things (some of which is already in this PR):

a. a global matcher, used as default when no matcher is specified. By default this should match dot files as the original code. I think using m_bHiddenFile makes sense in this context to improve performances across repeated calls.
b. a way to edit the global matcher (a dialog like the one in this PR - which I have not review yet btw). Changes would have global effect across TDE whenever the global matcher is used or when no matcher is used
c. a way to specify a custom matcher for a local application. This overrides the default matcher for the specific instance: for example we may want to use a different matcher in a Konqueror window or tab. This point implies having the ability to switch between global (default) matcher and custom matcher
d. a way to edit a custom matcher

I shall also discuss with @SlavekB when he is back from his trip early next week, but I am also interested in what you think about it.

In term of implementation, we can probably aim at implementing a. and b. first (I guess this PR does most of the job already) and later look at adding support for c. and d.

What do you think?

Hi @VinceR

As you can see, my KFileItem::isHidden(matcher) code uses this property to cache the most recent result of matcher->match(...). Why do that? Because there are other callers of this function that do NOT pass a TDEIO::HiddenFileMatcher object to it. Property m_bHiddenFile is for their benefit.

It is a very valid point and it raises two important questions.

How should we handle things from a global perspective?

How to set a global matcher?

The more I think about the functionality we are trying to implement, the more I see a need to have the following things (some of which is already in this PR):

a. a global matcher, used as default when no matcher is specified. By default this should match dot files as the original code. I think using m_bHiddenFile makes sense in this context to improve performances across repeated calls.
b. a way to edit the global matcher (a dialog like the one in this PR - which I have not review yet btw). Changes would have global effect across TDE whenever the global matcher is used or when no matcher is used
c. a way to specify a custom matcher for a local application. This overrides the default matcher for the specific instance: for example we may want to use a different matcher in a Konqueror window or tab. This point implies having the ability to switch between global (default) matcher and custom matcher
d. a way to edit a custom matcher

I shall also discuss with @SlavekB when he is back from his trip early next week, but I am also interested in what you think about it.

In term of implementation, we can probably aim at implementing a. and b. first (I guess this PR does most of the job already) and later look at adding support for c. and d.

What do you think?

@MicheleC

I think most of those requirements may already be satisfied.

A TDEIO::HiddenFileMatcher object always initializes to matching dotfiles on creation.
Any application can allocate and use its own TDEIO::HiddenFileMatcher object. It just needs to store / retrieve its application-specific match criteria string as a separate application-specific setting on disk. This PR is doing just that for a KonqListView paired with an associated KDirLister.
There is no reason why a global application-agnostic TDEIO::HiddenFileMatcher couldn't be allocated and used. The match criteria would be stored in a kdeglobals setting and modified by user through a control panel applet. The global defaults would be available by either using the global instance directly, or by initializing an application-specific matcher from the kdeglobals setting.
In fact, an earlier incarnation of my code did use single global matcher. I'll put together an example of how it might be implemented using the existing code.

> Hi @VinceR > > > As you can see, my KFileItem::isHidden(matcher) code uses this property to cache the most recent result of matcher->match(...). Why do that? Because there are other callers of this function that do NOT pass a TDEIO::HiddenFileMatcher object to it. Property m_bHiddenFile is for their benefit. > > It is a very valid point and it raises two important questions. > > 1. How should we handle things from a global perspective? > 2. How to set a global matcher? > > The more I think about the functionality we are trying to implement, the more I see a need to have the following things (some of which is already in this PR): > > a. a global matcher, used as default when no matcher is specified. By default this should match dot files as the original code. I think using `m_bHiddenFile` makes sense in this context to improve performances across repeated calls. > b. a way to edit the global matcher (a dialog like the one in this PR - which I have not review yet btw). Changes would have global effect across TDE whenever the global matcher is used or when no matcher is used > c. a way to specify a custom matcher for a local application. This overrides the default matcher for the specific instance: for example we may want to use a different matcher in a Konqueror window or tab. This point implies having the ability to switch between global (default) matcher and custom matcher > d. a way to edit a custom matcher > > I shall also discuss with @SlavekB when he is back from his trip early next week, but I am also interested in what you think about it. > > In term of implementation, we can probably aim at implementing a. and b. first (I guess this PR does most of the job already) and later look at adding support for c. and d. > > What do you think? @MicheleC I think most of those requirements may already be satisfied. 1. A `TDEIO::HiddenFileMatcher` object always initializes to matching dotfiles on creation. 2. Any application can allocate and use its own `TDEIO::HiddenFileMatcher` object. It just needs to store / retrieve its application-specific match criteria string as a separate application-specific setting on disk. This PR is doing just that for a `KonqListView` paired with an associated `KDirLister`. 3. There is no reason why a global application-agnostic `TDEIO::HiddenFileMatcher` couldn't be allocated and used. The match criteria would be stored in a kdeglobals setting and modified by user through a control panel applet. The global defaults would be available by either using the global instance directly, or by initializing an application-specific matcher from the kdeglobals setting. 4. In fact, an earlier incarnation of my code did use single global matcher. I'll put together an example of how it might be implemented using the existing code.

Yes, a global file matcher with settings in stored in kdeglobals is definetely good. Then we need to have a way to edit that from the TDE control panel.
For the application-specific (but even for the view-specific, think different tabs wanting to use different matchers) we will also need to provide a way to set them from a GUI. But this can come later.

@VinceR After adjusting the points we discussed so far, would you mind (force-)pushing the updated code before I continue the review? We have already many changes and it is better to realign on the code

@VinceR After adjusting the points we discussed so far, would you mind (force-)pushing the updated code before I continue the review? We have already many changes and it is better to realign on the code

MicheleC, I am almost done with changes and limited testing. Implementation of a single global hidden file matcher in addition to application-specific instances turned out to be trickier than I predicted so I need a bit more time with "real" system testing.

Give me a couple of days, then I will push updated code and we can proceed from there.

MicheleC, I am almost done with changes and limited testing. Implementation of a single global hidden file matcher in addition to application-specific instances turned out to be trickier than I predicted so I need a bit more time with "real" system testing.

Sure, no worries @VinceR. Take the time that is needed.
I would even suggest we break down this in multiple parts, if it is easier. something like this:

add global matcher and replicate current behavior for .dotfiles
add TCC module or GUI to be able to change the global matcher
add application specific matcher + GUI
We can then test and merge one part at a time.

@MicheleC,

This took somewhat longer than I expected but I finally have some updates for us to discuss. I think I hit just about every point you brought up. The new code is actively used/tested on my main system. I believe everything is working correctly, especially in conjunction with tdebase commit 8481f9c3f4.

That said, I am not quite satisfied with how things are implemented in KDirLister and KFileItem. After you have completed your next round of reviews of the current code, I want to discuss with you an idea wherein KFileItem maintains and uses its own HiddenFileMatcher* variable, initialized to the GlobalHiddenFileMatcher->getInstance(), and updated (once) by KDirlister to point to its own HiddenFileMatcher*. Doing this might eliminate the need for 2 variants of isHidden().

Thanks for the update @VinceR. I have to apologize in advance because most likely I will only be able to have a detail look at this next weekend, another busy work week ahead and some pending things to finish first :-(

@VinceR I am finally back to this PR. Will start reviewing from tomorrow. Sorry for the delay :-)

Hi @VinceR,

I want to discuss with you an idea wherein KFileItem maintains and uses its own HiddenFileMatcher* variable, initialized to the GlobalHiddenFileMatcher->getInstance(), and updated (once) by KDirlister to point to its own HiddenFileMatcher*. Doing this might eliminate the need for 2 variants of isHidden().

after reviewing the current code, I think your idea is good. KFileItems are created and used in many places, which makes dealing with them quite tricky. Your idea should help keep this simple, as long as we implement it carefully.
Moving forward, we should have something like this in place:

KFileItem to have an internal HiddenFileMatcher pointer.
Existing constructors of KFileItem to initialize such pointer to the global matcher object. This will allow the current TDE code to remain operational without disruption. Please keep the constructor syntax unchanged, for API/ABI compatibility.
the function isHidden() will use the current matcher to determine if an item is hidden or not. There is no more any need to have two different isHidden methods.
Add a new method to KFileItem API to allow to set a different matcher when necessary. This is not used yet, it will in future.

Following the model used for TDELocale, I suggest we proceed as follow for the global matcher:

make standalone h/cpp files for the HiddenFileMatcher class.
TDEGlobal to contain a pointer to the global instance of the matcher and will initialize it on the first usage (again, refer to TDELocale for an example)
the CommonHiddenFileMatcher class is unnecessary. TDEGlobal will do exactly the same thing and is already available to all applications in TDE.

Once all the above is in place, we can move to KDirLister and other places that use KFileItems and the matchers, including the GUI to set/modify the matcher in use. You may want to consider implementing the steps above as a new PR, to keep the code cleaner.

This PR is quite big, so let's address it and merge it step by step. Trying to do all in one go is a massive job and it helps to break it down in smaller chunks and verify that all is still working along the way.

Hi @VinceR, > I want to discuss with you an idea wherein KFileItem maintains and uses its own HiddenFileMatcher* variable, initialized to the GlobalHiddenFileMatcher->getInstance(), and updated (once) by KDirlister to point to its own HiddenFileMatcher*. Doing this might eliminate the need for 2 variants of isHidden(). after reviewing the current code, I think your idea is good. KFileItems are created and used in many places, which makes dealing with them quite tricky. Your idea should help keep this simple, as long as we implement it carefully. Moving forward, we should have something like this in place: 1. KFileItem to have an internal HiddenFileMatcher pointer. 2. Existing constructors of KFileItem to initialize such pointer to the global matcher object. This will allow the current TDE code to remain operational without disruption. Please keep the constructor syntax unchanged, for API/ABI compatibility. 3. the function `isHidden()` will use the current matcher to determine if an item is hidden or not. There is no more any need to have two different `isHidden` methods. 4. Add a new method to KFileItem API to allow to set a different matcher when necessary. This is not used yet, it will in future. Following the model used for TDELocale, I suggest we proceed as follow for the global matcher: 1. make standalone h/cpp files for the HiddenFileMatcher class. 2. TDEGlobal to contain a pointer to the global instance of the matcher and will initialize it on the first usage (again, refer to TDELocale for an example) 3. the CommonHiddenFileMatcher class is unnecessary. TDEGlobal will do exactly the same thing and is already available to all applications in TDE. Once all the above is in place, we can move to KDirLister and other places that use KFileItems and the matchers, including the GUI to set/modify the matcher in use. You may want to consider implementing the steps above as a new PR, to keep the code cleaner. This PR is quite big, so let's address it and merge it step by step. Trying to do all in one go is a massive job and it helps to break it down in smaller chunks and verify that all is still working along the way.

@MicheleC,

I am glad you were recently delayed in getting back to reviewing this PR because I also have been too busy to do any more work on it!. Your comments reflect what I have been thinking would be the next step. Here are some of my selective responses to them:

Add a new method to KFileItem API to allow to set a different matcher when necessary. This is not used yet, it will in future.

Actually we would use it right away to enable functionality for Konqueror listview (via associated KDirLister). But if you want to break up the PR, I guess we could save that part for later.

the CommonHiddenFileMatcher class is unnecessary. TDEGlobal will do exactly the same thing and is already available to all applications in TDE.

I don't quite follow this, but I still need to look at the TDELocale methodology. As long as there is a single pre-initialized instance of a "global" HiddenFileMatcher that other code can reference, it will all work just fine.

This PR is quite big, so let's address it and merge it step by step

That is probably a wise course of action. I guess I need to make another branch (just for myself for the while) that splits off the additional stuff I am doing in KDirLister.

This will take a bit longer while I do the necessary research, coding, and testing.

Many thanks for working with me on this,

Vince

the CommonHiddenFileMatcher class is unnecessary. TDEGlobal will do exactly the same thing and is already available to all applications in TDE.

I don't quite follow this, but I still need to look at the TDELocale methodology. As long as there is a single pre-initialized instance of a "global" HiddenFileMatcher that other code can reference, it will all work just fine.

Basically TDEGlobal will instantiate and provide access to a global HiddenFileMatcher. By default, this should provide the current behavior, that is filter out files that start with a dot. I haven't really commented about the HiddenFileMatcher class yet, although at a first glance I will some feedback to provide. Let me know if you prefer to have that now or after you create the new PR.

This PR is quite big, so let's address it and merge it step by step

That is probably a wise course of action. I guess I need to make another branch (just for myself for the while) that splits off the additional stuff I am doing in KDirLister.

Yes, probably wiser to use a different branch. We can keep this PR for reference, so the rest of the code is available while in the meantime we work through another branch step by step

Many thanks for working with me on this,

No worries, happy to work together and share knowledge. Any little bit helps to make TDE a better DE!!

@MicheleC,

I will be starting a new PR to simplify the code review and implementation process. The changes in that PR will be introduced incrementally as follows:

Implement 2 new classes in tdecore: a general purpose TDEStringMatcher and a TDEHiddenFileMatcher that inherits from it. Implement the first "user" of TDEHiddenFileMatcher as a single instance TDEGlobal::hiddenFileMatcher()
Modify KFileItem to use its own TDEHiddenFileMatcher* in isHidden(), initialized to the global instance and replaceable via an externally callable method. I am hoping to also retain and improve current match caching.
Modify KDirLister to use its own TDEHiddenFileMatcher* which it can pass on to each KFileItem as it gets created.

Since what I have in the current PR already works just fine, I feel compelled to retrofit any changes in the new PR back to this one for complete testing before I submit them for your review. That will cause some delay but I would feel better that you are reviewing code that actually works :)

I am ALMOST done with (1) and just need to do some testing. I am hoping to have the new PR ready for you by Wednesday.

@VinceR,
sounds like a good plan forward 👍 If I may suggest, it may be better to do 1. and 2. first, then test, merge and make sure all apps work as expected after a rebuild (they should) and then we move to 3.
Not sure why we need a TDEStringMatcher-TDEHiddenFileMatcher hierarchy, but will wait to see the code before further comments. If such structure is useful, we shall go for it.

I am hoping to have the new PR ready for you by Wednesday.

Take your time. It's another busy week at work, so most likely I won't review before the weekend anyway :-)

@VinceR,
sounds like a good plan forward 👍 If I may suggest, it may be better to do 1. and 2. first, then test, merge and make sure all apps work as expected after a rebuild (they should) and then we move to 3.

Since I am leaving on a weeklong vacation tomorrow, I won't have enough time to fully implement and test (2). In order to give you something to look at, I wanted to submit (1) since that is ready (and tested, albeit with all the other baggage in this PR).

If you want to postpone review until I get to (2), that's OK -- ETA will be 13 September. I suppose I also ought to add a proper control module as well.

Not sure why we need a TDEStringMatcher-TDEHiddenFileMatcher hierarchy, but will wait to see the code before further comments. If such structure is useful, we shall go for it.

Since TDEHiddenFileMatcher class provides a special purpose iterative string matching function, I thought it might be nice to also provide a more general class that applications can use for other purposes

I am hoping to have the new PR ready for you by Wednesday.

Take your time. It's another busy week at work, so most likely I won't review before the weekend anyway :-)

So instead of reviewing another PR, you can just relax :)

Hi @VinceR,
hope you are enjoying your holiday :-)

I am ok to review either (1) only or (1) and (2) together. In fact I thought you had pushed a PR with (1) only and wanted to review it today, but I can't seem to find it. I guess I may have misunderstood your prev comment.

Once you are back and ready, let me know and we resume the discussion.

Hi @MicheleC,

I am back and just created a new PR #178. I am hoping it will be easier to navigate than this one which ended up with a lot of RnD. Hopefully the commit messages and my introductory comments in the new PR will help you proceed with your review more easily.

VinceR

Hi Vince, will have a look thorugh the weekend, or earlier if find the time.

@VinceR
I started the review and there are lot of comments. I have published what done so far to make sure Firefox does not play tricks and lose all the comments made till now. I will continue the review tomorrow and if needed Monday, so please hold on till I complete it and I further ping you on this PR.

I hope the big amount of comments is not discouraging you, on the contrary that it serves as guidance for refining the solution.

@VinceR
to avoid keeping too many PR open and create confusion, is it ok if we mark this one as closed?
Also since the code has changed quite a bit with review on PR #178, would it be ok to delete the branch associated with this PR? This is mostly to keep the tdelibs repo as clean as possible. If you think the code in this PR is still required, we can leave the branch around for longer, no problem.

to avoid keeping too many PR open and create confusion, is it ok if we mark this one as closed?

Yes, go ahead and close the PR and remove the branch. I assume I will still be able to get back to the comments in this PR.

While branch issue/270/tdelibs is still the one I do active development & testing, I will not be doing any more pushes from it. Instead, I will selectively port changes to branch issue/270/tdelibs-V2 for your review.

Please keep related tdebase PR#273 open for the time being.

I assume I will still be able to get back to the comments in this PR.

Actually, this won't be an issue. I archived the web page & associated resources locally.

The comments and code remain available through gitea, so you can always come back here if you need to review things. In fact, the commits remain available in git too, if you checkout by hash. The only things that disappear is the branch pointer in git, which helps in keeping the repo cleaner.

    * whether or not an individual filesystem object is "hidden" by
    * current matcher criteria.
    */
   TDEIO::HiddenFileMatcher *matcher;

     virtual void setFilterDotFiles( bool filter );
     virtual void setFilterHiddenFiles( bool filter );
     /**
      * Checks whether filtering dot files is enabled.

      by the HiddenFileMatcher standards pointed to by @param matcher.
      Sets @property m_bHiddenFile accordingly.
    */
   void setHidden( TDEIO::HiddenFileMatcher *matcher );

       return m_url.fileName()[0] == '.';
   else // should never happen
       return m_strName[0] == '.';
   /* FIXME: Who calls this function? We are aware of the following callers:

    * True if file is "hidden" (i.e. hide-able)
      by current HiddenFileMatcher standards
    */
   bool m_bHiddenFile:1;

   /**
    * Pointer to object that encapsulates criteria for determining whether
    * or not a file is "hidden". There is one such object per KDirlister.


				`d = new KDirListerPrivate;`

				`matcher = new TDEIO::HiddenFileMatcher;`

   }
   delete d;
   delete matcher;

 }
 void KDirLister::setShowingDotFiles( bool _showDotFiles )
 void KDirLister::setShowingHiddenFiles( bool _showHiddenFiles )

     return false;
   if ( !d->isShowingDotFiles && item->isHidden() )
   const_cast<KFileItem*>(item)->setHidden( matcher );

         continue;
       }
       (*kit)->setHidden( matcher );

				`* This option is enabled by default.`
				`* @param filter true to enable filtering dot files, false to`

        && !isReadable())
      _state |= TDEIcon::LockOverlay;
   // kdWarning() << "KFileItem::overlays calling isHidden()" <<  endl;

				`}`


				`void KFileItem::setHidden( TDEIO::HiddenFileMatcher *matcher )`

   m_bLink( false ),
   m_bIsLocalURL( _url.isLocalFile() ),
   m_bMimeTypeKnown( false ),
   m_bHiddenFile( false ),

				`*/`
				`bool isHidden() const;`