Review Request: Review API of the new Path class

[Andreas Pakulat]

> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 92
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line92>
> >
> >     Is the string-conversion really needed? I would've hoped that most of the time we're dealing with KUrl's already.
> 
> Milian Wolff wrote:
>     Not if you operate on local files, e.g. Qt uses QString for local paths everywhere. Same for QFileInfo or IndexedString.

I guess the latter (IndexedString) is a bit of a problematic case, but for the others I'd say that whoever uses Path needs to do the conversion when it needs to access local files.

If it turns out that we end up with tons of this:

... (absfilename is a QString produced here somehow)
Path p(KUrl::fromPath(absfilename))

Then we can have a convenience constructor, but if thats not needed that often or a KUrl is being used in that place to get a local path for some Qt API and then later on a Path is constructed from the same thing, then I'd leave this constructor out.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 117
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line117>
> >
> >     What happens here when the subPath is a full url or a sub-path with multiple segments?
> 
> Milian Wolff wrote:
>     Sub path is expected to be a path, not a full url. The string will just be appended. Furthermore it does currently not support relative nor absolute subPaths. I'll add documentation and unit tests for that.
>     
>     Do you think these cases should be supported?

I think full url does not make any sense to support, but it should be tested and verified that it produces an invalid path. In fact while not being merged to master I'd probably add an assert for this case if it wouldn't break the tests.

Regarding passing in relative or absolute paths to generate a multi-segment path object, I don't know. If there are no use-cases right now documenting that subPath must not contain slashes (I'm assuming Path only deals with non-native paths which always use forward-slashes) and add docs and tests for that case as well. Once it becomes necessary you can still change the semantics for that particular case since nobody could have used it so far.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 193
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line193>
> >
> >     Is this really needed? I don't like transforming a Path to a String due to loosing all type information. I think we should only allow accessing individual path segments (like a list) or getting a fully-qualified KUrl out of the clas (and into it).
> 
> Milian Wolff wrote:
>     For local files this is much faster than a KUrl. And that is neat for checks in e.g. QFile::exists or for setting the text in a label or such.
>     
>     You sould take a look at the benchmarks, KUrl is just dog slow.

We could add such convenience API to Path itself if its used in many places, i.e. Path::exists(), Path::isDirectory(), Path::isFilename() (does a Path ever denote a relative path? Can't see how to construct that at the moment).

If you stick to a QString-getter, I'd probably go for a single one for now, i.e. toLocalFile and drop path and pathOrUrl. Then see how many places would actually not care about wether the Path is for a real url or a local file but need a QString and then decide to re-add whats needed.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 311
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line311>
> >
> >     Hmm, I think I'd separate the remote-url part from the path itself here. There are some places in the code where you have explicit checks for remote-urls and then take the first or second item from the list. Having a separate member would make that code a lot clearer.
> 
> Milian Wolff wrote:
>     I also thought about that but deliberately decided against it for space reasons (saving one pointer for local paths). But I agree that it might be a bit over-optimized. I'll revisit the code and take another look at it.

I see, but in this case I'd say optimization comes after code-readability. So once everythings ported we should measure the difference between saving the pointer or not and then decide wether saving it outweighs the special code in the various functions - at least IMO.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108196/#review24736
-----------------------------------------------------------


On Jan. 5, 2013, 10:59 a.m., Andreas Pakulat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108196/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 10:59 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Discuss API of the new Path class
> 
> 
> Diffs
> -----
> 
>   project/path.h PRE-CREATION 
>   project/path.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108196/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andreas Pakulat
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130105/cf231cb1/attachment-0001.html>