D20284: Fix l/100 km to MPG conversion
madcatx marked an inline comment as done. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D20284 To: madcatx, broulik, #frameworks, aacid Cc: apol, aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx marked 2 inline comments as done. madcatx added inline comments. INLINE COMMENTS > aacid wrote in fuel_efficiency.cpp:36 > I understand what you mean here, but i don't think that reciprocal is the > word that describes this (are you a native speaker? if so maybe it's juts > that my english is bad :D) > > Oh, it's actually called reciprocal number too, i think here we use the > inverse naming for it https://en.wikipedia.org/wiki/Multiplicative_inverse > > Maybe naming it "m_isReciprocaltoDefaultUnit" would make it understand? i.e. > makes it clear that that value is in relation to the default unit? Direct translation from my native language would be "inverse" too, although somehow I prefer the word "reciprocal" as it feels less ambiguous. (People may perceive "inversion" in multiple ways but "reciprocity" has a pretty solid definition as far as math goes... IMHO :) ) Regardless, I changed the name and added an explanatory comment. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D20284 To: madcatx, broulik, #frameworks, aacid Cc: apol, aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx updated this revision to Diff 56203. madcatx added a comment. More obvious explanation of the meaning of `m_isReciprocal` REPOSITORY R292 KUnitConversion CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20284?vs=56201=56203 REVISION DETAIL https://phabricator.kde.org/D20284 AFFECTED FILES autotests/convertertest.cpp src/fuel_efficiency.cpp To: madcatx, broulik, #frameworks, aacid Cc: apol, aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx updated this revision to Diff 56201. madcatx added a comment. Added an inverse test with initial value in MPG REPOSITORY R292 KUnitConversion CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20284?vs=55574=56201 REVISION DETAIL https://phabricator.kde.org/D20284 AFFECTED FILES autotests/convertertest.cpp src/fuel_efficiency.cpp To: madcatx, broulik, #frameworks, aacid Cc: apol, aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx marked an inline comment as done. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D20284 To: madcatx, broulik, #frameworks, aacid Cc: apol, aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx added inline comments. INLINE COMMENTS > apol wrote in fuel_efficiency.cpp:36 > I'm not sure I understand what isReciprocal means here. l/100 km (amount of fuel needed to cross a given distance) is reciprocal to MPG (distance crossed with a given amount of fuel). KUnitConverter can only scale units by a given factor, not invert them which is what is needed to make this work. `isReciprocal` flag denotes that the conversion between the base unit (l/100 km) and the target unit shall be reciprocal. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D20284 To: madcatx, broulik, #frameworks, aacid Cc: apol, aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx updated this revision to Diff 55574. madcatx added a comment. 1. Tweaked conversion factor to match those used by Google online converter 2. Added test to convert l/100 km to other representations and back REPOSITORY R292 KUnitConversion CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20284?vs=55512=55574 REVISION DETAIL https://phabricator.kde.org/D20284 AFFECTED FILES autotests/convertertest.cpp src/fuel_efficiency.cpp To: madcatx, broulik Cc: aacid, meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx updated this revision to Diff 55512. madcatx added a comment. Tabs vs spaces REPOSITORY R292 KUnitConversion CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20284?vs=55511=55512 REVISION DETAIL https://phabricator.kde.org/D20284 AFFECTED FILES src/fuel_efficiency.cpp To: madcatx, broulik Cc: meven, kde-frameworks-devel, michaelh, ngraham, bruns
D20284: Fix l/100 km to MPG conversion
madcatx created this revision. madcatx added a reviewer: broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. madcatx requested review of this revision. REVISION SUMMARY Previous code (probably) worked only for l/100 km -> MPG but not the other way around. Fix this by using reciprocal conversion when necessary. Output: 38.7 MPG -> 6.077 l/100 km; 16.455 km/l; 46.482 MPGI 16.455 km/l -> 6.077 l/100 km; 38.7 MPG 46.48 MPGI -> 6.077 l/100 km Seems good to me. Fixes bug 378967 REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D20284 AFFECTED FILES src/fuel_efficiency.cpp To: madcatx, broulik Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D15873: Improve debugging output by displaying whether the SMBSlave::del() function attempts to delete a file or a directory.
This revision was automatically updated to reflect the committed changes. Closed by commit R320:a26de31a5dfb: Distinguish between file and directory delete request in debug output. (authored by madcatx). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15873?vs=42646=43120 REVISION DETAIL https://phabricator.kde.org/D15873 AFFECTED FILES smb/kio_smb_dir.cpp To: madcatx, elvisangelaccio, broulik Cc: ngraham, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp
D15871: Add a specific error string for ENOTEMPTY return code in SMB slave.
This revision was automatically updated to reflect the committed changes. Closed by commit R320:75ee4dfb4042: Add a specific error string for ENOTEMPTY return code. (authored by madcatx). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15871?vs=42807=43119 REVISION DETAIL https://phabricator.kde.org/D15871 AFFECTED FILES smb/kio_smb_browse.cpp To: madcatx, elvisangelaccio, sitter, dfaure Cc: dfaure, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D15871: Add a specific error string for ENOTEMPTY return code in SMB slave.
madcatx updated this revision to Diff 42807. madcatx added a comment. Changed the KIO error code from ERR_INTERNAL to more specific ERR_CANNOT_RMDIR. REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15871?vs=42645=42807 REVISION DETAIL https://phabricator.kde.org/D15871 AFFECTED FILES smb/kio_smb_browse.cpp To: madcatx, elvisangelaccio, sitter Cc: dfaure, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D15871: Add a specific error string for ENOTEMPTY return code in SMB slave.
madcatx added a comment. Ah, sorry, apparently there is ERR_CANNOT_RMDIR and ERR_COULD_NOT_RMDIR which translate to the same value with the latter being deprecated. I'll update the patch to use ERR_CANNOT_RMDIR. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D15871 To: madcatx, elvisangelaccio, sitter Cc: dfaure, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D15871: Add a specific error string for ENOTEMPTY return code in SMB slave.
madcatx added a comment. Well, this (https://api.kde.org/frameworks/kio/html/deprecated.html#_deprecated15) marks KIO::ERR_COULD_NOT_RMDIR as deprecated. Is there an alternative I should use instead? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D15871 To: madcatx, elvisangelaccio, sitter Cc: dfaure, kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D15873: Improve debugging output by displaying whether the SMBSlave::del() function attempts to delete a file or a directory.
madcatx added a comment. @elvisangelaccio: OK, what do I have to do? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D15873 To: madcatx, elvisangelaccio, broulik Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D15873: Improve debugging output by displaying whether the SMBSlave::del() function attempts to delete a file or a directory.
madcatx created this revision. madcatx added a reviewer: elvisangelaccio. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. madcatx requested review of this revision. REVISION SUMMARY This has proven to be useful while I was tracking down what seems like a glitch in libsmbclient. Relevant Samba bug report: https://bugzilla.samba.org/show_bug.cgi?id=13637 REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D15873 AFFECTED FILES smb/kio_smb_dir.cpp To: madcatx, elvisangelaccio Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D15871: Add a specific error string for ENOTEMPTY return code in SMB slave.
madcatx created this revision. madcatx added a reviewer: elvisangelaccio. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. madcatx requested review of this revision. REVISION SUMMARY ENOEMPTY return code may be a valid return code from i.e. smbc_rmdir(). The current error reporting code lets such an error code fall through to the default report case, leading to confusing information reported back to the user. This patch adds such a case branch. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D15871 AFFECTED FILES smb/kio_smb_browse.cpp To: madcatx, elvisangelaccio Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx added a comment. What is the status on upstreaming this? Is there anything else you need from me to finish this off? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: cfeck, rdieter, graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx added inline comments. INLINE COMMENTS > cfeck wrote in kio_smb_browse.cpp:521 > While it does no harm, you do not need 'const' for plain types, such as 'int'. Don't worry, I'm well aware. I however like to use consts wherever possible to make it clear that changing a value of such variable makes no logical sense: It's a way of telling "You shouldn't ever have to change this and if you do, perhaps you should think again about what you're trying to do." REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: cfeck, rdieter, graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx added a comment. In https://phabricator.kde.org/D8387#157778, @ngraham wrote: > Do you have commit access? If not, I'll be happy to commit this once some more of the folks who have made comments have also signed off on a final version. No I don't. elvisangelaccio already commited a few patches on my behalf, can you please make sure you use the same e-mail address as before to commit this? Thanks. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: rdieter, graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx marked 10 inline comments as done. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: rdieter, graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx updated this revision to Diff 21049. madcatx added a comment. - Check against a range of affected versions - Small stylistic changes REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8387?vs=21044=21049 REVISION DETAIL https://phabricator.kde.org/D8387 AFFECTED FILES smb/kio_smb.cpp smb/kio_smb.h smb/kio_smb_browse.cpp To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx added inline comments. INLINE COMMENTS > elvisangelaccio wrote in kio_smb_browse.cpp:521 > Don't we set `m_enableEEXISTWorkaround` in the constructor? Yes. The idea is this: When the slave loads, it checks what libsmbclient lib is available and sets the `m_enableEEXISTWorkaround` if a broken version is detected. Then iff the return code from `smbc_opendir()` is EEXIST and the `m_enableEEXISTWorkaround` is true EEXIST is treated as EACCES or EPERM. This should hopefully prevent any weird behavior when EEXIST might be a valid return code. Do you still think that the method that does this "apply the workaround" logic could use a better name? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx added inline comments. INLINE COMMENTS > elvisangelaccio wrote in kio_smb.cpp:67 > While at it, I'd move `m_openFd` on its own line It kind of violates my "change only as much as is needed" but the whole kio-smb source could probably use thorough reformatting anyway. Will do. > elvisangelaccio wrote in kio_smb_browse.cpp:521 > Maybe call it `needsWorkaroundEEXIST()`? > > Current name seems to imply the method //is// the workaround, while it just > tells us whether we need the workaround. Well, this method actually applies the workaround. The `m_enableEEXISTWorkaround` is set to true when the an affected libsmbclient library is detected. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx updated this revision to Diff 21044. madcatx added a comment. Tabs/spaces issue REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8387?vs=21040=21044 REVISION DETAIL https://phabricator.kde.org/D8387 AFFECTED FILES smb/kio_smb.cpp smb/kio_smb.h smb/kio_smb_browse.cpp To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx updated this revision to Diff 21040. madcatx edited the summary of this revision. madcatx added a comment. Detect versions of libsmbclient libraries that are considered broken and apply the workaround only for those. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8387?vs=21028=21040 REVISION DETAIL https://phabricator.kde.org/D8387 AFFECTED FILES smb/kio_smb.cpp smb/kio_smb.h smb/kio_smb_browse.cpp To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: graesslin, z3ntu
D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7
madcatx added a comment. In https://phabricator.kde.org/D8387#157502, @ngraham wrote: > So am I correct that this patch does the following: > > - If you go to a real password-protected samba server, without this patch you are totally screwed; with it, you are correctly offered a chance to enter your credentials > - If you go to an invalid address that doesn't exist, without this patch you are told that; with it, you are erroneously offered a credentials window, making you think that the address is real > > If so, is there any other way to check whether the server address is valid besides getting back EEXIST? This is pretty much the case. One way how to get the login dialog without the patch is to manually enter a valid path like this: "smb://SERVER/valid_share/valid_directory". In this case libsmbclient returns EPERM instead of EEXIST if the requested directory is password-protected. I believe I descried this in a bit more detail in the Samba bug report. In https://phabricator.kde.org/D8387#157551, @graesslin wrote: > Could you check which libsmbclient version is used and ifdef the change? Ifdefing around this won't do much good because applications should not require a rebuild against updated libsmbclient. Peeking through the header file I discovered `const char * smbc_version(void)`. I'll see if I can use this to restrict this hack only to the troublesome libsmbclient versions. Stay tuned. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D8387 To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks Cc: graesslin, z3ntu
