D12696: Use the new uds implementation

jtamate added a comment. I'm sorry, I was missing a -dev package and therefore I was not compiling kio sftp. A fix for this problem is in D13475 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #framewo

D12696: Use the new uds implementation

bruns added a comment. In D12696#276696 , @bruns wrote: > `UDS_FILE_TYPE = 13`: 32768 -> 16384 (010 -> 04) > File mode changes from 'regular file' to 'directory' https://github.com/KDE/kio-extras/blob/master/sftp/kio_sftp.cpp#L

D12696: Use the new uds implementation

bruns added a comment. In D12696#276660 , @martinkostolny wrote: > `udsField=33554445, value=16384` `33554445 = 0x20d = UDS_NUMBER = 0x0200 | UDS_FILE_TYPE = 13` https://api.kde.org/frameworks/kio/html/classKIO_1_1UDSEntry.ht

D12696: Use the new uds implementation

martinkostolny added a comment. I've managed to get more info about the crash also by following this guide: https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves#Attaching_gdb_to_an_io-slave Here is backtrace from gdb: F5905112: udsentry_bt.txt

D12696: Use the new uds implementation

lbeltrame added a comment. I wonder if this is related to timeline:/ no longer working properly in the file dialog. (didn't investigate yet). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #frameworks Cc: lbeltrame, asturmlechner, martinkosto

D12696: Use the new uds implementation

asturmlechner added a comment. No crash in my case, but this commit makes links with sftp:/ inaccessible. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #frameworks Cc: asturmlechner, martinkostolny, kde-frameworks-devel, bruns, michaelh, ngr

D12696: Use the new uds implementation

martinkostolny added a comment. Thanks for guidance regarding getting additional debug info. I'll try to get it soon :). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #frameworks Cc: martinkostolny, kde-frameworks-devel, bruns, michaelh, ngr

D12696: Use the new uds implementation

bruns added inline comments. INLINE COMMENTS > martinkostolny wrote in udsentry.cpp:106 > sftp slave crashes here when entering directory with links Any chance you can attach a debugger (or load the core file with gdb) and provide the values of 'udsField' and 'value'? REPOSITORY R241 KIO RE

D12696: Use the new uds implementation

jtamate added a comment. In D12696#275105 , @martinkostolny wrote: > Hi! Probably after this commit sftp slave crashes when showing a directory with links. Please see my code comment. Can you also reproduce or is it on my side only? I'm

D12696: Use the new uds implementation

martinkostolny added a comment. Hi! Probably after this commit sftp slave crashes when showing a directory with links. Please see my code comment. Can you also reproduce or is it on my side only? Here is a crash log: log_kio_sftp: readdir: "/mnt/ext/files" , details: "2" ASS

D12696: Use the new uds implementation

This revision was automatically updated to reflect the committed changes. Closed by commit R241:e80c31163170: Use the new uds implementation (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12696?vs=33929&id=33957 REVISION DETAIL https://ph

D12696: Use the new uds implementation

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, bruns, michaelh, ngraham

D12696: Use the new uds implementation

jtamate updated this revision to Diff 33929. jtamate marked 8 inline comments as done and 3 inline comments as done. jtamate edited the summary of this revision. jtamate added a comment. Added the documentation for insert. Removed the () from the QDataStream& operators, but must be kept for Q

D12696: Use the new uds implementation

dfaure added a comment. Starting to look good ;-) INLINE COMMENTS > udsentry.h:143 > /** > * insert field with string value > * @param field numeric field id The old insert() should be documented that it asserts if the entry already exists, and that one should use replace()

D12696: Use the new uds implementation

bruns added inline comments. INLINE COMMENTS > bruns wrote in kfileitem.cpp:198 > Wouldn't it be better to call m_entry.clear() and m_entry.insert(...) here? > > For the call from the constructor, m_entry is empty, so you save the > std::find_if() in the replace method calls. > > For any other

D12696: Use the new uds implementation

bruns added inline comments. INLINE COMMENTS > kfileitem.cpp:198 > const QByteArray pathBA = QFile::encodeName(path); > if (QT_LSTAT(pathBA.constData(), &buf) == 0) { > +m_entry.replace(KIO::UDSEntry::UDS_DEVICE_ID, > buf.st_dev); m_entry.rese

D12696: Use the new uds implementation

jtamate updated this revision to Diff 33889. jtamate edited the summary of this revision. jtamate added a comment. Restricted Application added a subscriber: kde-frameworks-devel. Direct friendship with the operators. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.o

D12696: Use the new uds implementation

jtamate added inline comments. INLINE COMMENTS > dfaure wrote in udsentry.cpp:454 > Hmm why can't this be the friend function directly? > > I don't like the added global functions in the public header... > Hmm why can't this be the friend function directly? Because the compiler (clang++ in thi

D12696: Use the new uds implementation

dfaure added inline comments. INLINE COMMENTS > udsentry.cpp:454 > > -KIOCORE_EXPORT QDebug operator<<(QDebug stream, const KIO::UDSEntry &entry) > +KIOCORE_EXPORT QDataStream &operator<<(QDataStream &s, const UDSEntry &a) > { Hmm why can't this be the friend function directly? I don't like

D12696: Use the new uds implementation

jtamate updated this revision to Diff 33815. jtamate added a comment. Make some friends functions. A bad type in copy&paste didn't allowed me the use of the module functions save/load/debugUDSEntry, REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12696?vs=338

D12696: Use the new uds implementation

jtamate updated this revision to Diff 33809. jtamate added a comment. Don't use java style. As UDSEntryPrivate is private, declare public methods save, load and debugUDSEntry, otherwise I couldn't access the private d pointer from the << and >> operators. REPOSITORY R241 KIO CHANGES SIN

D12696: Use the new uds implementation

dfaure added a comment. Oh OK I had misunderstood your comment. Right, those methods don't have to be inline. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #frameworks Cc: bruns, michaelh, ngraham

D12696: Use the new uds implementation

bruns added a comment. In D12696#259292 , @dfaure wrote: > I requested it, for proper encapsulation Whats the difference between class UDSEntryPrivate { void load(...) { // function body } }; and

D12696: Use the new uds implementation

dfaure added a comment. I requested it, for proper encapsulation REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate, dfaure, #frameworks Cc: bruns, michaelh, ngraham

D12696: Use the new uds implementation

bruns added inline comments. INLINE COMMENTS > udsentry.cpp:132 > +} > +static void save(QDataStream &s, const UDSEntry &a) > +{ Is there a reason to define the methods inside the class declaration? The diff would be significantly smaller if you kept the definition separate. REPOSI

D12696: Use the new uds implementation

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > udsentry.cpp:132 > +} > +static void save(QDataStream &s, const UDSEntry &a) > +{ strange that it's static, "a.d" could be this, if it wasn't stat

D12696: Use the new uds implementation

jtamate updated this revision to Diff 33758. jtamate marked 9 inline comments as done. jtamate edited the summary of this revision. jtamate added a comment. Hopefully, all the issues fixed. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12696?vs=33622&id=33758

D12696: Use the new uds implementation

dfaure added a comment. First line of commit log should have changelog in mind, so more context needed. Maybe rather something like "KIO::UDSEntry: switch to a single std::vector for more performance". REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D12696 To: jtamate,

D12696: Use the new uds implementation

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Looks good. INLINE COMMENTS > udsentry.cpp:45 > +inline Field(const uint index, long long value = 0) : m_long(value), > m_index(index) {} > +// This operator help

D12696: Use the new uds implementation

jtamate created this revision. jtamate added reviewers: dfaure, Frameworks. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY This new implementation is the fastest in the autotests. The replaceOrInsert in KFileItem::setName is due