shubham updated this revision to Diff 44710.
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16249?vs=44688&id=44710
REVISION DETAIL
https://phabricator.kde.org/D16249
AFFECTED FILES
src/core/copyjob.cpp
src/core/global.h
src/core/job_error.cpp
To: shubha
ngraham added a comment.
"Format with destination drive to a filesystem type which" -> "Reformat the
destination drive to use a filesystem that"
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks, bruns
Cc: cfeck, br
shubham updated this revision to Diff 44688.
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16249?vs=43938&id=44688
REVISION DETAIL
https://phabricator.kde.org/D16249
AFFECTED FILES
src/core/copyjob.cpp
src/core/global.h
src/core/job_error.cpp
To: shubha
bruns added a comment.
Please update the diff.
INLINE COMMENTS
> bruns wrote in job_error.cpp:1079
> "Format **with**"
> "filesystem type" or just "filesystem"
> either "a file" or better "files"
"transeferred"
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To:
bruns requested changes to this revision.
This revision now requires changes to proceed.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks, bruns
Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
shubham added a comment.
Can I get a review?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
shubham added a comment.
@bruns Apart from these, does the code seems sane to you?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
bruns added inline comments.
INLINE COMMENTS
> job_error.cpp:1078
> +description = xi18n("The file %1 cannot be
> transeferred,"
> +" because the the destination's filesystem does
> not support files that large", errorText);
> +solutions << i18n("Form
shubham updated this revision to Diff 43938.
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16249?vs=43930&id=43938
REVISION DETAIL
https://phabricator.kde.org/D16249
AFFECTED FILES
src/core/copyjob.cpp
src/core/global.h
src/core/job_error.cpp
To: shubha
shubham added a comment.
@ngraham No, please recheck, the check is before the first m_totalSize >
m_freeSpace check
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: cfeck, bruns, kde-frameworks-devel, michaelh,
ngraham added a comment.
`1ul << 32) -1` needs a comment explaining what it means, for people like me
with puny brains. :)
There are //two// checks in this file that result in an `ERR_DISK_FULL` if
there is not enough space. The FAT32 check is now before the second one, but
not the firs
shubham updated this revision to Diff 43930.
shubham marked 5 inline comments as done.
shubham added a comment.
File size limit check for a given file system before (m_totalSize >
m_freeSpace) check
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16249?vs=43895
shubham added inline comments.
INLINE COMMENTS
> ngraham wrote in job_error.cpp:253
> Unrelated and incorrect change: `https` is correct here, so please revert.
That was my previous commit where I changed http -> https. Don't know why it
affected my local branch.
REPOSITORY
R241 KIO
REVISIO
ngraham added a comment.
Tested this out and couldn't actually make it work the way I asked--with the
FAT32 file size limit checked first. I think I see why: there are two `total
size > available space?` checks that output `ERR_DISK_FULL` on error, and both
of them execute first. The FAT32 f
bruns added inline comments.
INLINE COMMENTS
> cfeck wrote in copyjob.cpp:1622
> Does this even work on a -m32 system?
`(1ul << 32) -1` then ...
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: cfeck, bruns, kde-
bruns added inline comments.
INLINE COMMENTS
> job_error.cpp:248
> break;
> +case KIO::ERR_FILE_TOO_LARGE_FOR_FAT32:
> +result = i18n("The file named %1 cannot be transferred because its
> size is greater than 4 GB,"
Should also be handled in `KIO::rawErrorDetail(...)`, sam
cfeck added inline comments.
INLINE COMMENTS
> bruns wrote in copyjob.cpp:1622
> why not `1 << 32 - 1` ?
> Also note the `- 1`, 2 ^ 32 is 0, as FAT32 uses a uint32_t for the file
> size.
Does this even work on a -m32 system?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.or
ngraham added inline comments.
INLINE COMMENTS
> bruns wrote in job_error.cpp:249
> %1 with quotes please, as it can contain whitespace, so it would be hard to
> spot where the filename ends.
> I would also prefer a sentence where the reason is given first, e.g.
> `The destination filesystem onl
bruns added inline comments.
INLINE COMMENTS
> copyjob.cpp:1622
> +
> +if (fileSystem == KFileSystemType::Fat && (*it).size >
> 4294967296) { // 4 GB = 4294967296 Bytes
> +q->setError(ERR_FILE_TOO_LARGE_FOR_FAT32);
why not `1 << 32 - 1` ?
Also note the `- 1`, 2 ^ 32
ngraham added inline comments.
INLINE COMMENTS
> global.h:249
> +ERR_CANNOT_CREATE_SLAVE = KJob::UserDefinedError + 73, ///< used by
> Slave::createSlave, @since 5.30
> +ERR_FILE_TOO_LARGE_FOR_FAT32 = KJob::UserDefinedError + 74
> };
Please add a comment with `@since` to this
> job_er
ngraham added a comment.
Thank you for indulging me 😊
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
shubham updated this revision to Diff 43895.
shubham retitled this revision from "Warn user before copy/move job if the file
size exceeds the maximum possible file size(4 GB) in FAT32 file system" to
"Warn user before copy/move job if the file size exceeds the maximum possible
file size in FAT32
22 matches
Mail list logo
