Replace deprecated UDSEntry::insert with UDSEntry::fastInsert
Needs RevisionPublic

Authored by gengisdave on Sep 9 2018, 2:34 PM.

Details

Reviewers
nmel
Group Reviewers
Krusader
Summary

With KIO 5.48 UDSEntry::insert is deprecated in favor of UDSEntry::fastInsert.

Test Plan

Compiled and tested on KIO 5.50 - zero warnings

TODO - Compile and test with a KIO < 5.48 - zero warnings expected too

No changes are expected is Krusader use : classic file browsing, iso and krarc behaviours should not change

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
gengisdave requested review of this revision.Sep 9 2018, 2:34 PM
gengisdave created this revision.
nmel requested changes to this revision.Sep 12 2018, 6:20 AM
nmel added a subscriber: nmel.

inline

iso/iso.cpp
190

Check major version too. This will break with 6.0 release.

krArc/krarc.cpp
1509

I propose to reduce code duplication and declare macros INSERT = fastInsert if KIO_VERSION_MINOR >= 48 and insert for the other case.

Then use

entry.INSERT(...

This way is cleaner IMO.

This revision now requires changes to proceed.Sep 12 2018, 6:20 AM

The use of a Macro was my second way to solve this, but I thought it would add more complexity to the code; instead, this patch makes the code unclear to read.

Where to put the macro? The three files don't have a common header file and I haven't found a suitable .h file; any advice?

nmel added a comment.Sep 13 2018, 5:48 AM

Where to put the macro? The three files don't have a common header file and I haven't found a suitable .h file; any advice?

It doesn't fit any existing header. I would put it in the krusader/compat.h.

BTW, proper check for version seems to be KIO_VERSION >= QT_VERSION_CHECK(5, 48, 0).

asensi added a subscriber: asensi.EditedSep 13 2018, 6:14 PM

Thanks Davide! (and Nikita :-) )

this patch makes the code unclear to read.

What do you think about creating a specialized version of UDSEntry, e.g. KrUDSEntry? This way the code:

void kio_isoProtocol::createUDSEntry(const KArchiveEntry * isoEntry,  UDSEntry & entry)
{
    entry.clear();
#if KIO_VERSION_MINOR >= 48
    entry.fastInsert(UDSEntry::UDS_NAME, isoEntry->name());
    entry.fastInsert(UDSEntry::UDS_FILE_TYPE, isoEntry->permissions() & S_IFMT);   // keep file type only
    entry.fastInsert(UDSEntry::UDS_ACCESS, isoEntry->permissions() & 07777);   // keep permissions only
#else
    entry.insert(UDSEntry::UDS_NAME, isoEntry->name());
    entry.insert(UDSEntry::UDS_FILE_TYPE, isoEntry->permissions() & S_IFMT);   // keep file type only
    entry.insert(UDSEntry::UDS_ACCESS, isoEntry->permissions() & 07777);   // keep permissions only
#endif
[...]

would still be clear and maintainable, e.g.:

void kio_isoProtocol::createUDSEntry(const KArchiveEntry * isoEntry, KrUDSEntry & entry)
{
    entry.clear();
    entry.insert(UDSEntry::UDS_NAME, isoEntry->name());
    entry.insert(UDSEntry::UDS_FILE_TYPE, isoEntry->permissions() & S_IFMT);   // keep file type only
    entry.insert(UDSEntry::UDS_ACCESS, isoEntry->permissions() & 07777);   // keep permissions only
[...]

and the macros would only be seen inside void KrUDSEntry::insert().


This is a draft:

krudsentry.h:

#ifndef KRUDSENTRY_H
#define KRUDSENTRY_H

#include <KIO/UDSEntry>

class KrUDSEntry : public KIO::UDSEntry
{
public:
    void insert(uint field, const QString &value);
    void insert(uint field, long long l);
};

#endif // KRUDSENTRY_H

krudsentry.cpp:

#include "krudsentry.h"

#include <kio_version.h>

void KrUDSEntry::insert(uint field, const QString &value)
{
#if KIO_VERSION >= QT_VERSION_CHECK(5, 48, 0)
    UDSEntry::fastInsert(field, value);
#else
    UDSEntry::insert(field, value);
#endif
}

void KrUDSEntry::insert(uint field, long long l)
{
#if KIO_VERSION >= QT_VERSION_CHECK(5, 48, 0)
    UDSEntry::fastInsert(field, l);
#else
    UDSEntry::insert(field, l);
#endif
}

What do you think about creating a specialized version of UDSEntry, e.g. KrUDSEntry? This way the code:

This is an elegant solution, but it require to change all occurence of UDSEntry entry with the new KrUDSEntry entry; I'll take some time to make some tests.

gengisdave updated this revision to Diff 41936.Sep 19 2018, 2:15 PM

Changed in favor of a macro solution (as suggested by Nikita).

I'm still exploring the method proposed by Toni, but I've found some problems in krarc.

nmel added a comment.Sep 22 2018, 5:31 AM

Thanks Davide! This looks less cluttered.

Since INSERT becomes kind of "global" macro, I suggest we rename it to UDS_ENTRY_INSERT, since the insert word is too generic while the implementation is not. Thanks!

krusader/compat.h
28

Do we really need UDSEntry:: here?

asensi added a comment.Oct 2 2018, 5:13 PM

Since INSERT becomes kind of "global" macro, I suggest we rename it to UDS_ENTRY_INSERT, since the insert word is too generic while the implementation is not. Thanks!

It looks like it's a more adequate name for that macro.

gengisdave updated this revision to Diff 43419.Oct 11 2018, 8:20 PM

I thought I had already committed that... changed to a more human macro name

nmel added a comment.Oct 16 2018, 6:27 AM

Looks much better, thanks!

krusader/compat.h
27

INSERT -> UDS_ENTRY_INSERT in this file

nmel requested changes to this revision.Oct 27 2018, 7:52 AM

Kindly ping.

This revision now requires changes to proceed.Oct 27 2018, 7:52 AM