Review Request 115499: Season of KDE Project - KRFB

George Kiagiadakis kiagiadakis.george at gmail.com
Mon Feb 17 21:32:01 UTC 2014 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115499/#review50035
-----------------------------------------------------------


Fairly good quality code, although there are some issues here and there...


krfb/invitationsrfbclient.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35195>

    I don't like very much that this code gets duplicated from RfbClient here. Why do you need this here?



krfb/invitationsrfbserver.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35205>

    There is a configuration option in the settings that allows the user to disable publishing the service on the local network. This should be checked and respected here.



krfb/invitationsrfbserver.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35198>

    This is not very good, but I can accept it. In general wallet operations should be done asynchronously, because there are many bugs related to synchronous wallet operations (crashes, deadlocks...). Ideally the passwords should be saved as soon as the user changes them in the UI, and they should be saved asynchronously.



krfb/mainwindow.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35200>

    No fixed sizes please. Using a different style, not everything fits in the window and it looks incredibly ugly. The window should have a size hint, but it should be allowed to grow and shrink, either automatically or by the user.



krfb/mainwindow.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35202>

    The parent should be "this", not "0". This is to make sure that the message box is centered inside the krfb window.



krfb/mainwindow.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35201>

    This is interfering with the fact that RfbServerManager is also using the aboutToQuit() signal to delete all the servers, including InvitationsRfbServer. You don't know which one will be executed first and this is bad...



krfb/mainwindow.cpp
<https://git.reviewboard.kde.org/r/115499/#comment35203>

    Also, the parent should be this.



krfb/rfbserver.h
<https://git.reviewboard.kde.org/r/115499/#comment35192>

    Making stop() a virtual method introduces a small problem there. In the destructor of RfbServer (~RfbServer), stop() is being called. However, the instance of the subclass gets destroyed before the superclass gets destroyed, so when stop() is called from ~RfbServer(), only the base class implementation (RfbServer::stop()) is being run, not the subclass one. This can potentially introduce problems. The solution is to call stop() before destroying the object.



krfb/ui/mainwidget.ui
<https://git.reviewboard.kde.org/r/115499/#comment35184>

    Again, no fixed sizes.



libvncserver/main.c
<https://git.reviewboard.kde.org/r/115499/#comment35139>

    I'd rather avoid this change. Can't we force clients to disconnect from somewhere else in the krfb code? We should avoid changing libvncserver, as it is an external library and I was hoping that I could remove it from krfb and use the upstream one bundled with distributions. The only reason it exists here is because I needed to patch it a couple of years ago, but all my patches are now in version 0.9.9 upstream, so I want to remove it if possible.



libvncserver/sockets.c
<https://git.reviewboard.kde.org/r/115499/#comment35138>

    Is this really needed? What does it do?  For the same reason (libvncserver code), we should avoid it.


Some more issues that I have noticed:

* The password edit button inititially says "Edit" with letters, but if you click on it it becomes a floppy icon and when you save the password it becomes the "document-edit" icon. For consistency, it should also initially have the "document-edit" icon instead of saying "Edit".

* I cannot enable the rfb server at all. It always returns an error, not sure why.

* krfb crashes on exit inside ~InvitationsRfbServer. In the backtrace it looks like the destructor is being recursively called from inside the destructor. I haven't understood why yet.


- George Kiagiadakis


On Feb. 5, 2014, 3:51 p.m., Amandeep Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115499/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2014, 3:51 p.m.)
> 
> 
> Review request for Telepathy and George Kiagiadakis.
> 
> 
> Repository: krfb
> 
> 
> Description
> -------
> 
> Season of KDE Project - KRFB
> 
> Status Quo
> -----------
> Currently KRFB works on an invite-only model, where the host user needs to create an invite via the Invitation Dialog.
> This provides the connection details (address and password), which must then be communicated to the remote user.
> No information about telepathy contacts with whom desktop can be shared is present in the KRFB Dialog.
> The implementation of RFB also lacks support of clipboard operations like cut and paste of text.
> 
> Changes
> -------
> Krfb modified to move away from invite-model, and modeled as a service that can be started/stopped.
> Bidirectional text-clipboard operations are supported.
> Telepathy contacts with rfb capability shown inside main window, with an option to share desktop.
> Desktop sharing password visible in main window, with option to change it.
> Unattended Access allowed with separate password.
> Fixed bugs in libvncserver responsible for KRFB crashing.
> Krfb now runs as KUniqueApplication.
> 
> All changes are present in the following clone of KRFB
> 
> Repository
> ----------
> http://quickgit.kde.org/?p=clones%2Fkrfb%2Famandeepsingh%2Fkrfb-sok.git
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 78c19b3 
>   cmake/modules/FindKTp.cmake PRE-CREATION 
>   krfb/CMakeLists.txt bbc508d 
>   krfb/invitation.h 27dddeb 
>   krfb/invitation.cpp 5379060 
>   krfb/invitationmanager.h a611e48 
>   krfb/invitationmanager.cpp b9abe94 
>   krfb/invitationsrfbclient.h f263f64 
>   krfb/invitationsrfbclient.cpp bd5cff8 
>   krfb/invitationsrfbserver.h 9faecb6 
>   krfb/invitationsrfbserver.cpp 47ed934 
>   krfb/invitedialog.h 3e45bb1 
>   krfb/invitedialog.cpp e911fdb 
>   krfb/krfb.desktop 41d0fae 
>   krfb/krfb.kcfg acae17f 
>   krfb/main.cpp 8803baa 
>   krfb/mainwindow.h PRE-CREATION 
>   krfb/mainwindow.cpp PRE-CREATION 
>   krfb/manageinvitationsdialog.h 3740ebc 
>   krfb/manageinvitationsdialog.cpp 2c786a3 
>   krfb/pendingrfbclient.h c0524fe 
>   krfb/pendingrfbclient.cpp c16b045 
>   krfb/personalinvitedialog.h caadbb9 
>   krfb/personalinvitedialog.cpp 2851678 
>   krfb/rfbclient.h 7b46d3f 
>   krfb/rfbclient.cpp 9ab16eb 
>   krfb/rfbserver.h 6ef73e4 
>   krfb/rfbserver.cpp cd5d6c0 
>   krfb/trayicon.cpp 02ae7af 
>   krfb/tubesrfbclient.h 3908e47 
>   krfb/tubesrfbclient.cpp 2466826 
>   krfb/tubesrfbserver.h 596636c 
>   krfb/tubesrfbserver.cpp 575c3cd 
>   krfb/ui/configsecurity.ui 4afbb58 
>   krfb/ui/mainwidget.ui PRE-CREATION 
>   krfb/ui/manageinvitations.ui f727af6 
>   krfb/ui/personalinvitewidget.ui 5d12c2a 
>   libvncserver/main.c b47e873 
>   libvncserver/sockets.c e54252d 
> 
> Diff: https://git.reviewboard.kde.org/r/115499/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Amandeep Singh
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20140217/89302018/attachment-0001.html>

More information about the KDE-Telepathy mailing list