Review Request 120986: fix Bug 340470 - Unable to filter for opening square brackets: [

[Emmanuel Pescosta]

> On Nov. 4, 2014, 4:03 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp, lines 46-62
> > <https://git.reviewboard.kde.org/r/120986/diff/1/?file=325322#file325322line46>
> >
> >     Maybe we can make this code easier by always using the m_regExp to check if the pattern is a valid regexp or not.
> >     
> >     All this code could be replaced by:
> >     
> >     if (!m_regExp) {
> >        ...
> >     }
> >     m_regExp->setPattern(filter);
> >     m_useRegExp = m_regExp->isValid();
> >     
> >     What do you think?
> 
> Arjun AK wrote:
>     That won't work because even "foo" is a valid regexp
> 
> Frank Reininghaus wrote:
>     Do we really use regular expression matching at the moment? AFAIK, there is only wildcard matching (i.e., '?' matches a single character and an asterisk any string). I don't know why the check if the filter pattern contains '[' is done at all.
> 
> Emmanuel Pescosta wrote:
>     > Do we really use regular expression matching at the moment?
>     
>     Unix shell wildcards also support [] as far as I know (we use QRegExp::WildcardUnix here which supports Unix shell wildcards).
> 
> Frank Reininghaus wrote:
>     You're right, sorry for the noise: http://qt-project.org/doc/qt-4.8/qregexp.html#wildcard-matching
> 
> Emmanuel Pescosta wrote:
>     > That won't work because even "foo" is a valid regexp
>     
>     Ah yes makes sense ... sorry.
>     
>     What about adding "(filter.contains('[') && filter.contains(']'))" to the or-chain of m_useRegExp and after setting the pattern of m_regExp we reset the m_useRegExp to m_regExp->isValid(). 
>     So we can avoid the temporary reg exp and we guarantee that the reg exp is only used when it is valid (like your current patch already does :).
> 
> Arjun AK wrote:
>     >What about adding "(filter.contains('[') && filter.contains(']'))" to the or-chain of m_useRegExp and after setting the pattern of m_regExp we reset the m_useRegExp to m_regExp->isValid().
>     So we can avoid the temporary reg exp and we guarantee that the reg exp is only used when it is valid (like your current patch already does :).
>     
>     That would mean we are performing related operations (changing the value of `m_useRegExp`) in two different places, instead of one.
> 
> Emmanuel Pescosta wrote:
>     I would do it this way: http://goo.gl/VAW8en
> 
> Arjun AK wrote:
>     >I would do it this way: http://goo.gl/VAW8en
>     
>     That looks OK. But isn't stack allocation very cheap?

It was about parsing the filter string twice when there is a squared bracket in it.
This can be avoided by using the m_regExp directly.


- Emmanuel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120986/#review69820
-----------------------------------------------------------


On Nov. 9, 2014, 8:50 a.m., Arjun AK wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120986/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2014, 8:50 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> We need to make sure input is a valid regexp before enabling regexp filter mode.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/private/kfileitemmodelfilter.cpp 2e320f2 
> 
> Diff: https://git.reviewboard.kde.org/r/120986/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arjun AK
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20141109/157a098a/attachment.htm>