D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
shubham updated this revision to Diff 44710. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16249?vs=44688=44710 REVISION DETAIL https://phabricator.kde.org/D16249 AFFECTED FILES src/core/copyjob.cpp src/core/global.h src/core/job_error.cpp To: shubham, ngraham, elvisangelaccio, #frameworks, bruns Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
shubham updated this revision to Diff 44688. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16249?vs=43938=44688 REVISION DETAIL https://phabricator.kde.org/D16249 AFFECTED FILES src/core/copyjob.cpp src/core/global.h src/core/job_error.cpp To: shubham, ngraham, elvisangelaccio, #frameworks, bruns Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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("Format the destination drive to a filesystem
> format which supports file that large.");
double "the"
drop the "`s"
> job_error.cpp:1079
> +" because the the destination's filesystem does
> not support files that large", errorText);
> +solutions << i18n("Format the destination drive to a filesystem
> format which supports file that large.");
> +break;
"Format **with**"
"filesystem type" or just "filesystem"
either "a file" or better "files"
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
shubham updated this revision to Diff 43938. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16249?vs=43930=43938 REVISION DETAIL https://phabricator.kde.org/D16249 AFFECTED FILES src/core/copyjob.cpp src/core/global.h src/core/job_error.cpp To: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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 first one, so the first one still will still look for adequate space before checking to see whether or not the copy is even possible due to FAT32 file size restrictions. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D16249 To: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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=43930 REVISION DETAIL https://phabricator.kde.org/D16249 AFFECTED FILES src/core/copyjob.cpp src/core/global.h src/core/job_error.cpp To: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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 REVISION DETAIL https://phabricator.kde.org/D16249 To: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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 file size limit check should be before the first one, probably near `// Stat the next src url`. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D16249 To: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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(...)`, same file below.
> ngraham wrote in job_error.cpp:249
> None of the other error messages use quotes around the filenames. What's most
> semantically correct would be to change `i18n` to `xi18n` and wrap the
> filename/path token in `` tags (this is exactly what
> they're for). We can do that in this patch, and then change it for all the
> other error messages (as appropriate) in another patch.
>
> Also, in terms of wording, the HIG recommends putting the most important part
> of the sentence first, which in this case would be the error message; the
> explanation would go second. See
> https://hig.kde.org/style/writing/wording.html. So the current sentence may
> be a bit long, but it's structurally correct as-is.
`"Could not transfer %1 because it is to large. The destination filesystem only
supports files up to 4GB.` is shorter ...
The second part may even be removed alltogether, see `KIO::rawErrorDetail`,
https://github.com/KDE/kio/blob/master/src/core/job_error.cpp#L311
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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.org/D16249 To: shubham, ngraham, elvisangelaccio, #frameworks Cc: cfeck, bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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 only supports files up to 4GB. "%1" is to large > an can not be transferred.` None of the other error messages use quotes around the filenames. What's most semantically correct would be to change `i18n` to `xi18n` and wrap the filename/path token in `` tags (this is exactly what they're for). We can do that in this patch, and then change it for all the other error messages (as appropriate) in another patch. Also, in terms of wording, the HIG recommends putting the most important part of the sentence first, which in this case would be the error message; the explanation would go second. See https://hig.kde.org/style/writing/wording.html. So the current sentence may be a bit long, but it's structurally correct as-is. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D16249 To: shubham, ngraham, elvisangelaccio, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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 is 0, as FAT32 uses a uint32_t for the file size.
> job_error.cpp:249
> +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,"
> + " and the destination's filesystem does not support
> files that large.", errorText);
%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 only supports files up to 4GB. "%1" is to large an
can not be transferred.`
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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_error.cpp:253
> default:
> -result = i18n("Unknown error code %1\n%2\nPlease send a full bug
> report at https://bugs.kde.org.;, errorCode, errorText);
> +result = i18n("Unknown error code %1\n%2\nPlease send a full bug
> report at http://bugs.kde.org.;, errorCode, errorText);
> break;
Unrelated and incorrect change: `https` is correct here, so please revert.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D16249
To: shubham, ngraham, elvisangelaccio, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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
D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 file system(4 GB)
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 file system(4 GB) ". REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16249?vs=43894=43895 REVISION DETAIL https://phabricator.kde.org/D16249 AFFECTED FILES src/core/copyjob.cpp src/core/global.h src/core/job_error.cpp To: shubham, ngraham, elvisangelaccio, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
