kio-stash is in KDE Review

[Arnav Dhamija]

Hi everyone

I have touched upon any concerns people have had with the ioslave in
my previous email. If no one has any objections, I would like to ship
this to KDE Extra Modules. More specifically, it will be suited for
kde-extragear-utils, if that division is still relevant in KDE
extragear. The sysadmin task for the same can be found here:
https://phabricator.kde.org/T6285

On Sat, May 27, 2017 at 3:43 PM, Arnav Dhamija <arnav.dhamija at gmail.com> wrote:
> First off, thanks for all the reviews!
>
>>
>> We simulate a "virtual://" protocol[*] which can contain virtual
>> folders containing references to real files and folders.
>
>
> I did not know that such a feature exists! It seems like this feature is
> Krusader specific without tapping into KIO as such?
>
>> It would be great if we can get rid of this "Krusader only" solution and
>> replace it with
>> your *real* KIO protocol. Some questions: Is it persistent (between
>> reboots)? And does it
>> support general file URLs (e.g. ftp://localhost/path/to/file.txt) or only
>> local
>> "file://"-files?
>
>
> The list of files is not persistent across reboots. That idea was scrapped
> fairly early on during the development of this project. It could probably be
> implemented using xattr if such a feature is needed. I haven't tested much
> on other protocols, but it does have problems copying from some protocols
> such as mtp.
>
> That said, being a kioslave, it works well with Dolphin, the KDE Folder view
> Plasmoid, and Konqueror for file operations.
>
>> * ../src/iodaemon/stashnotifier.cpp:183:9: warning: variable 'fileType' is
>> uninitialized when used here
>> This should probably be fixed.
>>
>> * You are linking to KI18n but you are not using i18n() calls in your
>> code. Have a look at [1].
>>
>> * The dbus adaptor could probably use build-time generation via cmake,
>> rather than being committed to the git repo. If you need an example look at
>> the CMakeLists.txt in kio/src/kioexec (qt5_add_dbus_adaptor and friends).
>
>
>  Thanks, I have fixed the first two issues. My response to the last issue is
> given below.
>
>> Files that contain autogenerated code should ideally not be "edited".
>>
>> Do you remember why editing was needed?
>
>
>> Not exactly, Arnav will know that. But it's not really too big a
>> problem for a project this size. In fact I do the same thing in
>> Spectacle.
>
>
> I believe we needed to make some specific changes to the autogenerated XML
> at some point in the project. These changes were later scrapped, so it uses
> the directly autogenerated code with no modifications. As Boudhayan said,
> the justification for not changing it to be invoked on compilation was for
> simplicity and that the D-Bus code was rarely updated throughout the
> development of the project.
>
>> Maintainers will leave and we will inherit the code, so there will be
>> noone to
>> ask.
>
>
> I will be available to maintain this code when it gets released in the KDE
> software ecosystem.
>
>
> On Sat, May 27, 2017 at 2:53 PM, Albert Astals Cid <aacid at kde.org> wrote:
>>
>> El dissabte, 27 de maig de 2017, a les 0:29:51 CEST, Boudhayan Gupta va
>> escriure:
>> > Hi,
>> >
>> > On 27 May 2017 at 00:20, Albert Astals Cid <aacid at kde.org> wrote:
>> > > El divendres, 26 de maig de 2017, a les 23:48:18 CEST, Boudhayan Gupta
>> > > va
>> > >
>> > > escriure:
>> > >> Hi,
>> > >>
>> > >> On 26 May 2017 at 20:31, Elvis Angelaccio <elvis.angelaccio at kde.org>
>> wrote:
>> > >> > * The dbus adaptor could probably use build-time generation via
>> > >> > cmake,
>> > >> > rather than being committed to the git repo. If you need an example
>> > >> > look
>> > >> > at
>> > >> > the CMakeLists.txt in kio/src/kioexec (qt5_add_dbus_adaptor and
>> > >> > friends).
>> > >>
>> > >> IIRC we did that first, but then figured we needed to edit the
>> > >> generated code (I was the mentor for this project).
>> > >
>> > > That seems dangerous, what if sometime in the future you need to
>> > > regenerate
>> > > the adaptor because you add new functions or something?
>> > >
>> > > Files that contain autogenerated code should ideally not be "edited".
>> > >
>> > > Do you remember why editing was needed?
>> >
>> > Not exactly, Arnav will know that. But it's not really too big a
>> > problem for a project this size. In fact I do the same thing in
>> > Spectacle.
>> >
>> > Granted this isn't best practice, but the effort required to do it
>> > right (whatever way that might be) was inordinately large for the
>> > scope of the project. At some point someone who's trying to patch this
>> > part of the code might get stuck, but figuring out that generated code
>> > was edited isn't difficult at all, as neither is asking the maintainer
>> > how the code works.
>>
>> Maintainers will leave and we will inherit the code, so there will be
>> noone to
>> ask.
>>
>> If you need to do some changes to generated code either document it or
>> just
>> don't do changes in the generated code.
>>
>> Cheers,
>>   Albert
>>
>> >
>> > Thanks,
>> > Boudhayan
>> >
>> > > Cheers,
>> > >
>> > >   Albert
>> > >
>> > >> > Cheers,
>> > >> > Elvis
>> > >> >
>> > >> > [1]: https://api.kde.org/frameworks/ki18n/html/prg_guide.html
>> > >>
>> > >> Freundliche Grüße
>> > >> Boudhayan Gupta
>> > >> KDE e.V. - Sysadmin and Community Working Groups
>> > >> +49 151 71032970
>>
>>
>
>
>
> --
> arnav dhamija



-- 
arnav dhamija