Hello,
Thanks for the review!
The only thing I noticed is that it doesn't implement replacement of
images using ftp protocol. Could you please add support for
ftp?
Well, I'm afraid that adding support for ftp is not a good idea because
unlike http, the ftp protocol does not support methods like getContentType.
There, you see a given resource as a file and you have no idea what you are
actually downloading so it might not even be an image.
In net.java.sip.communicator.impl.gui.main.chat.ShowPreviewDialog
class msgIDToChatString and msgIDandPositionToLink
properties are not private and they are used directly (without setters
and getters) in ChatConversationPannel class. I don't think it's
a big deal but I think it will be better if you use methods for adding
and removing elements from them.
Since net.java.sip.communicator.impl.gui.main.chat.ShowPreviewDialog
and ChatConversationPannel
are in the same package I think it is better to leave thinks here as they
are. Adding accessors to fields that are used only within the package seems
unnecessary to me. Of course I could be wrong. What the others think about
this?
Could you please add comments for confService and bundleContext
properties from net.java.sip.communicator.impl.replacement.
directimage.DirectImageActivator class?
Done. Thanks for noticing.
I noticed that in DirectImageActivator class in start method there are
two service registrations assigned to
directImageSourceServReg. Maybe this is a copy paste mistake.
No, this is not by mistake. Adding two service registrations is necessary
because this implementation must be associated with two service interfaces
- ReplacementService and DirectImageReplacementService.
You can notice that the same thing is done in the smiley service activator.
Once again thanks for the great review!
If you have any further commenteries on the subject I will be glad to
discuss them!
Regards,
Marin
image preview.patch (43.8 KB)
···
On Thu, Sep 26, 2013 at 5:43 PM, Hristo Terezov <hristo@jitsi.org> wrote:
Hello,
I tried your patch and it works great! The only thing I noticed is that it
doesn't implement replacement of images using ftp protocol. Could you
please add support for ftp?
I also have little comments about the code.
In net.java.sip.communicator.impl.gui.main.chat.ShowPreviewDialog
class msgIDToChatString and msgIDandPositionToLink properties are not
private and they are used directly (without setters and getters) in
ChatConversationPannel class. I don't think it's a big deal but I think it
will be better if you use methods for adding and removing elements from
them.
Could you please add comments for confService and bundleContext properties
from net.java.sip.communicator.impl.replacement.directimage.DirectImageActivator
class?
I noticed that in DirectImageActivator class in start method there are two
service registrations assigned to directImageSourceServReg. Maybe this is a
copy paste mistake.
If you have any questions please contact me.
Regards,
Hristo.
On Mon, Sep 23, 2013 at 5:42 PM, Marin Dzhigarov <marin@bluejimp.com>wrote:
Hello,
Emil asked me to make some modifications here so I'm attaching a new
patch for a review. Please discard the old one.
Regards,
Marin
On Thu, Sep 19, 2013 at 1:37 PM, Marin Dzhigarov <marin@bluejimp.com>wrote:
Hello,
I'm sending you this patch for a review.
It contains some modifications on the replacement services and the
classes that use them.
-Basically, if replacement of Image/Video is disabled a (show preview)
link will be added after Image/Video links so that the user can quickly
re-enable the replacement services.
-Some modifications were made in several replacement services so that
the actual replacement images will be obtained over https instead of http.
-DirectImageReplacementService now checks via the http headers whether
the provided source link actually contains an image resource and how big
the size of that image is because it can potentially be extremely large.
Any comments on the topic will be appreciated.
Regards,
Marin
_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev