bruns added inline comments.
INLINE COMMENTS
> hallas wrote in fstabhandling.cpp:374
> @bruns - I am little confused now. You wrote previously that you could see
> the point in the `id()` function, but now you want it moved?
> What kind of unit test are you missing? I can't quite follow the
hallas marked an inline comment as done.
hallas added inline comments.
INLINE COMMENTS
> bruns wrote in fstabhandling.cpp:374
> The `id()` method and members are still part of the FstabEntry, where they do
> not belong.
>
> The UDIs go through some further mangling in
bruns added inline comments.
INLINE COMMENTS
> hallas wrote in fstabhandling.cpp:374
> No problem, I really appreciate the thorough review feedback you provide. I
> definitely prefer to take more time and get things right, then to rush things
> and potentially get them wrong.
>
> So, have you
hallas marked an inline comment as done.
hallas added a comment.
Hi @bruns - did you have a chance to go through this patch again? Am I
missing anything to move on with this?
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D27152
To: hallas, #frameworks, bruns, meven
meven added inline comments.
INLINE COMMENTS
> bruns wrote in fstabhandling.cpp:209
> add temporary for fstype
The temporary for fstype is gone it seems.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D27152
To: hallas, #frameworks, bruns, meven
Cc:
hallas marked 4 inline comments as done.
hallas added inline comments.
INLINE COMMENTS
> bruns wrote in fstabhandling.cpp:374
> For now just keep it here, I can see now benefit of making it public (even
> unexported).
>
> And thanks for baring with my nitpicking ;-), this now is pretty much
bruns added inline comments.
INLINE COMMENTS
> hallas wrote in fstabhandling.cpp:374
> I guess the main reason is that the id is a function of a `FilesystemEntry`,
> meaning that the id is computed from the contents of it. You are absolutely
> right that it is not picked up directly from the
hallas added inline comments.
INLINE COMMENTS
> bruns wrote in fstabhandling.cpp:374
> Why is the id() a property of the entry now?
>
> - it does not correspond to anything from the fstab/mtab
> - it is only used here to generate the key for the cache
>
> Please move the id generation back
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> fstabhandling.cpp:374
> +const FilesystemEntry fsEntry(fsname, mountpoint, type,
> QStringList());
> +
hallas updated this revision to Diff 76990.
hallas marked 7 inline comments as done.
hallas added a comment.
Review comments
REPOSITORY
R245 Solid
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D27152?vs=76898=76990
BRANCH
introduce_filesystem_entry
REVISION DETAIL
hallas added inline comments.
INLINE COMMENTS
> bruns wrote in filesystem_entry.cpp:31
> This is a behavioral change:
>
> - overlayfs is no longer handled
> - fuse.cryfs is no longer handled
>
> - encfs gets an extra '@', and the prefix is changed from to
>
> The identifier is API, don't
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filesystem_entry_test.h:6
> + * it under the terms of the GNU General Public License as published by *
> + * the Free Software Foundation; either version 2
hallas updated this revision to Diff 76898.
hallas marked 9 inline comments as done.
hallas added a comment.
Addressed review comments
REPOSITORY
R245 Solid
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D27152?vs=75961=76898
BRANCH
introduce_filesystem_entry
REVISION DETAIL
hallas added a comment.
@bruns - thanks a lot for the feedback, I have updated the patch with your
suggestions.
INLINE COMMENTS
> bruns wrote in filesystem_entry.cpp:77
> Why don't you just keep the string?
It was merely meant as an optimization, I don't know if it makes sense - I'll
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filesystem_entry.cpp:77
> +{
> +return m_type == FilesystemType::Unknown ? m_typeString :
> FilesystemTypeToString(m_type);
> +}
Why don't you just keep
hallas added a comment.
Hi @bruns - I have updated this patch with the changes you requested and
would really like your feedback :)
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D27152
To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n,
meven accepted this revision.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D27152
To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
hallas added inline comments.
INLINE COMMENTS
> meven wrote in filesystem_entry.cpp:47
> This sound sane to me.
> Ideally we would store only the string in case of an unknown fs or the enum
> but not both for a same entry, the choice could be done in ctor.
I have added a new enum class type
hallas updated this revision to Diff 75961.
hallas marked 17 inline comments as done.
hallas added a comment.
Updated patch with review comments
REPOSITORY
R245 Solid
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D27152?vs=74977=75961
BRANCH
introduce_filesystem_entry
meven added inline comments.
INLINE COMMENTS
> hallas wrote in filesystem_entry.cpp:47
> Yeah I agree ;) But I would really like for this class to be used more
> generically so having a list of filesystems here would probably make it hard
> for that. We could also have two methods, one similar
bruns added inline comments.
INLINE COMMENTS
> CMakeLists.txt:3
> +filesystem_entry.cpp
> +filesystem_entry.h
> fstabmanager.cpp
remove
> filesystem_entry.cpp:59
> +{
> +if (m_type == QLatin1Literal("fuse.encfs")) {
> +return m_device + QLatin1Char('@') + m_mountPath;
bruns requested changes to this revision.
This revision now requires changes to proceed.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D27152
To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
hallas added inline comments.
INLINE COMMENTS
> meven wrote in filesystem_entry.cpp:47
> It would be great to expose an enum (KIO's KFileSystemType does this a
> little).
> But given the number of filesystems this days, it may be overcomplicated.
Yeah I agree ;) But I would really like for
meven accepted this revision.
meven added a comment.
This revision is now accepted and ready to land.
Nice one. Good step splitting out this.
INLINE COMMENTS
> filesystem_entry.cpp:47
> +
> +QString FilesystemEntry::type() const
> +{
It would be great to expose an enum (KIO's
hallas created this revision.
hallas added reviewers: Frameworks, bruns, meven.
hallas added a project: Frameworks.
hallas requested review of this revision.
REVISION SUMMARY
Introdue a dedicated type for representing a FilesystemEntry. A
FilesystemEntry is either mounted by the system or can
25 matches
Mail list logo
