Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated May 2, 2015, 4:46 p.m.) Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK. Changes --- fixed issues Bugs: 341773 https://bugs.kde.org/show_bug.cgi?id=341773 Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs (updated) - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated May 2, 2015, 7:26 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK. Changes --- Submitted with commit c806f88e4ea65330719fa1721cdf15ea5cbddb5a by Ashish Bansal to branch master. Bugs: 341773 https://bugs.kde.org/show_bug.cgi?id=341773 Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review79767 --- Don't remove the case that improves things for users :) src/core/global.cpp (line 394) https://git.reviewboard.kde.org/r/123224/#comment54633 should be declared here (QString nameSuffix = ...) src/core/global.cpp (line 398) https://git.reviewboard.kde.org/r/123224/#comment54632 oldName.mid(0) is the same as oldName. src/core/global.cpp (line 402) https://git.reviewboard.kde.org/r/123224/#comment54634 prepend modifies the string already, no need for nameSuffix = src/core/global.cpp (line 412) https://git.reviewboard.kde.org/r/123224/#comment54635 mid(0, start) is more commonly written as left(start). Same for the other mid(0, ) above. The unittest will tell you if I'm wrong by one char, but I don't think so :) - David Faure On April 30, 2015, 9:05 a.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 30, 2015, 9:05 a.m.) Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK. Bugs: 341773 https://bugs.kde.org/show_bug.cgi?id=341773 Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 30, 2015, 9:05 a.m.) Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK. Changes --- fixed issues Bugs: 341773 https://bugs.kde.org/show_bug.cgi?id=341773 Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs (updated) - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
On April 28, 2015, 2:35 p.m., David Faure wrote:
src/core/global.cpp, line 407
https://git.reviewboard.kde.org/r/123224/diff/3/?file=363223#file363223line407
startsWith('.') (using the QChar overload)
Do we even need this special case, with the way the code is now? It
seems to me that removing this first if() branch would work just the same.
Ashish Bansal wrote:
The first if() has been used for getting . (1).txt instead of .txt (1)
But yeah, code looks nasty with this special case. Should I remove this?
- Ashish
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123224/#review79629
---
On April 30, 2015, 9:05 a.m., Ashish Bansal wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123224/
---
(Updated April 30, 2015, 9:05 a.m.)
Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK.
Bugs: 341773
https://bugs.kde.org/show_bug.cgi?id=341773
Repository: kio
Description
---
For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong
name(something like filename-1 2.6.tar.gz).
Expected name: filename-1.6 (1).tar.gz
Diffs
-
autotests/globaltest.cpp ff8725d
src/core/global.cpp f18ac10
Diff: https://git.reviewboard.kde.org/r/123224/diff/
Testing
---
Works fine!
Thanks,
Ashish Bansal
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 28, 2015, 2:38 p.m.) Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK. Changes --- (David) (but Arjun's patch was then reverted) -- also add bug number. Bugs: 341773 https://bugs.kde.org/show_bug.cgi?id=341773 Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123224/#review79631
---
src/core/global.cpp (line 396)
https://git.reviewboard.kde.org/r/123224/#comment54446
you can change that for:
if (oldName.lastIndexOf('.') == 0)
src/core/global.cpp (line 412)
https://git.reviewboard.kde.org/r/123224/#comment54447
Also char overload.
- Aleix Pol Gonzalez
On April 28, 2015, 4:38 p.m., Ashish Bansal wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123224/
---
(Updated April 28, 2015, 4:38 p.m.)
Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Arjun AK.
Bugs: 341773
https://bugs.kde.org/show_bug.cgi?id=341773
Repository: kio
Description
---
For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong
name(something like filename-1 2.6.tar.gz).
Expected name: filename-1.6 (1).tar.gz
Diffs
-
autotests/globaltest.cpp ff8725d
src/core/global.cpp f18ac10
Diff: https://git.reviewboard.kde.org/r/123224/diff/
Testing
---
Works fine!
Thanks,
Ashish Bansal
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123224/#review79629
---
src/core/global.cpp (line 396)
https://git.reviewboard.kde.org/r/123224/#comment54440
startsWith('.') (using the QChar overload)
Do we even need this special case, with the way the code is now? It seems
to me that removing this first if() branch would work just the same.
src/core/global.cpp (line 398)
https://git.reviewboard.kde.org/r/123224/#comment54437
oldName.mid(1), faster (and more readable) than section with an empty
separator.
src/core/global.cpp (line 399)
https://git.reviewboard.kde.org/r/123224/#comment54435
isEmpty() rather than isNull(), not point in being specific about the
difference between the two.
src/core/global.cpp (line 402)
https://git.reviewboard.kde.org/r/123224/#comment54436
Faster: nameSuffix.prepend('.') (using the QChar overload).
src/core/global.cpp (line 403)
https://git.reviewboard.kde.org/r/123224/#comment54442
oldName.left(...) or .mid(0, ...)
... I'm not even sure what section(empty string, ...) really does :-)
src/core/global.cpp (line 414)
https://git.reviewboard.kde.org/r/123224/#comment54443
does not exists - does not exist
src/core/global.cpp (line 415)
https://git.reviewboard.kde.org/r/123224/#comment5
basename +=
src/core/global.cpp (line 417)
https://git.reviewboard.kde.org/r/123224/#comment54445
QString suggestedName =
(it's not used before this line, so it should be declared here; could even
be const QString suggestedName = ... since it's not modified later)
- David Faure
On April 26, 2015, 12:19 p.m., Ashish Bansal wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123224/
---
(Updated April 26, 2015, 12:19 p.m.)
Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez.
Repository: kio
Description
---
For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong
name(something like filename-1 2.6.tar.gz).
Expected name: filename-1.6 (1).tar.gz
Diffs
-
autotests/globaltest.cpp ff8725d
src/core/global.cpp f18ac10
Diff: https://git.reviewboard.kde.org/r/123224/diff/
Testing
---
Works fine!
Thanks,
Ashish Bansal
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
On April 6, 2015, 1:12 p.m., Martin Klapetek wrote: autotests/globaltest.cpp, line 96 https://git.reviewboard.kde.org/r/123224/diff/2/?file=359784#file359784line96 This should be (1).txt? Ashish Bansal wrote: imho if my any file starts with ., that's my dot file and I would not like suggestName to remove the dotness of file instead it should append/increment no. at end. But if you still find no. at the starting as better use case, then no problem I'll change it right away. David Faure wrote: I agree, a dot file should keep starting with a dot. Martin Klapetek wrote: Indeed. I'd suggest .(1).txt maybe to also keep the file extension in tact? Yeah that sounds good. Changed it to . (1).txt :) - Ashish --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review78559 --- On April 26, 2015, 12:19 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 26, 2015, 12:19 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
On April 25, 2015, 7:42 p.m., David Faure wrote: src/core/global.cpp, line 397 https://git.reviewboard.kde.org/r/123224/diff/2/?file=359785#file359785line397 Why do you assemble a list, and then only look at the last element? You just need an int and a QMimeType, if only the last element matters. Also, why loop over every character? Is the goal to handle .foo.txt vs .tar.gz? There's API for that in QMimeDatabase, it can return the mimetype and even the known suffix without any iteration needed, see QMimeDatabase::suffixForFileName. Ah sorry, I didn't know that QMimeDatabase::suffixForFileName can handle .foo.txt vs .tar.gz :) - Ashish --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review79509 --- On April 26, 2015, 12:19 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 26, 2015, 12:19 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 26, 2015, 12:19 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Changes --- fixed issues Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs (updated) - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
On April 6, 2015, 1:12 p.m., Martin Klapetek wrote: autotests/globaltest.cpp, line 96 https://git.reviewboard.kde.org/r/123224/diff/2/?file=359784#file359784line96 This should be (1).txt? Ashish Bansal wrote: imho if my any file starts with ., that's my dot file and I would not like suggestName to remove the dotness of file instead it should append/increment no. at end. But if you still find no. at the starting as better use case, then no problem I'll change it right away. I agree, a dot file should keep starting with a dot. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review78559 --- On April 4, 2015, 1:15 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 4, 2015, 1:15 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review79509 --- src/core/global.cpp (line 396) https://git.reviewboard.kde.org/r/123224/#comment54323 Why do you assemble a list, and then only look at the last element? You just need an int and a QMimeType, if only the last element matters. Also, why loop over every character? Is the goal to handle .foo.txt vs .tar.gz? There's API for that in QMimeDatabase, it can return the mimetype and even the known suffix without any iteration needed, see QMimeDatabase::suffixForFileName. - David Faure On April 4, 2015, 1:15 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 4, 2015, 1:15 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
On April 6, 2015, 3:12 p.m., Martin Klapetek wrote: autotests/globaltest.cpp, line 96 https://git.reviewboard.kde.org/r/123224/diff/2/?file=359784#file359784line96 This should be (1).txt? Ashish Bansal wrote: imho if my any file starts with ., that's my dot file and I would not like suggestName to remove the dotness of file instead it should append/increment no. at end. But if you still find no. at the starting as better use case, then no problem I'll change it right away. David Faure wrote: I agree, a dot file should keep starting with a dot. Indeed. I'd suggest .(1).txt maybe to also keep the file extension in tact? - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review78559 --- On April 4, 2015, 3:15 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 4, 2015, 3:15 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review78559 --- Just a question - why is the current suggested name wrong? autotests/globaltest.cpp (line 96) https://git.reviewboard.kde.org/r/123224/#comment53746 This should be (1).txt? - Martin Klapetek On April 4, 2015, 3:15 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 4, 2015, 3:15 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
On April 6, 2015, 1:12 p.m., Martin Klapetek wrote: Just a question - why is the current suggested name wrong? In name filename-1.6.tar.gz, 1.6 denotes package version and new suggested name changes it to filename-1 2.6.tar.gz. It should add 1 after the package version and should not increment package name from 1.6 to 2.6. If i keep calling suggestName, it changes it further to 3.6, 4.6 and so on. So, I hope that explains the current suggested name is wrong :) On April 6, 2015, 1:12 p.m., Martin Klapetek wrote: autotests/globaltest.cpp, line 96 https://git.reviewboard.kde.org/r/123224/diff/2/?file=359784#file359784line96 This should be (1).txt? imho if my any file starts with ., that's my dot file and I would not like suggestName to remove the dotness of file instead it should append/increment no. at end. But if you still find no. at the starting as better use case, then no problem I'll change it right away. - Ashish --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/#review78559 --- On April 4, 2015, 1:15 p.m., Ashish Bansal wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 4, 2015, 1:15 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123224: KIO::suggestName suggests wrong name for some filenames
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123224/ --- (Updated April 4, 2015, 1:15 p.m.) Review request for KDE Frameworks, Plasma and Aleix Pol Gonzalez. Repository: kio Description --- For filenames like filename-1.6.tar.gz, KIO::suggestName suggests wrong name(something like filename-1 2.6.tar.gz). Expected name: filename-1.6 (1).tar.gz Diffs - autotests/globaltest.cpp ff8725d src/core/global.cpp f18ac10 Diff: https://git.reviewboard.kde.org/r/123224/diff/ Testing --- Works fine! Thanks, Ashish Bansal ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
