[jitsi-dev] PATCH: Replacement of Image/Video links

Marin_Dzhigarov1 · 2013-09-19 10:37:18 UTC

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

image preview .patch (37.2 KB)

Marin_Dzhigarov1 · 2013-09-23 14:42:31 UTC

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

image preview.patch (43.7 KB)

···

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

Hristo_Terezov · 2013-09-26 14:43:52 UTC

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

Marin_Dzhigarov1 · 2013-09-27 06:37:39 UTC

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

damencho · 2013-09-27 07:06:17 UTC

Hi,

> 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?

The practical example: one day some check need to be added when
accessing msgIDToChatString,
but there is no get method, and by that time 3 more developers have used
the class and for a simple check a lot of code needs to be changed and a
lot of checking to make sure it is not used in other places ...
It is a good practice to have accessors, and that's how all of the code is
done. And we need to keep code consistent that is the reason all of the
code (700k+ lines :)) look like this after 10 years.
And as code convention page says, when uncertain ask, so thanks Marin and
Hristo for bringing it up.

Cheers
damencho

···

On Fri, Sep 27, 2013 at 9:37 AM, Marin Dzhigarov <marin@bluejimp.com> wrote:

Hristo_Terezov · 2013-09-27 09:40:23 UTC

Hello,

I talked with Emil about the ftp use case that we can check the file type
by the file's extension. We can replace only the files that have image
extension.

···

On Fri, Sep 27, 2013 at 10:06 AM, Damian Minkov <damencho@jitsi.org> wrote:

Hi,

On Fri, Sep 27, 2013 at 9:37 AM, Marin Dzhigarov <marin@bluejimp.com>wrote:

> 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?

The practical example: one day some check need to be added when accessing msgIDToChatString,
but there is no get method, and by that time 3 more developers have used
the class and for a simple check a lot of code needs to be changed and a
lot of checking to make sure it is not used in other places ...
It is a good practice to have accessors, and that's how all of the code is
done. And we need to keep code consistent that is the reason all of the
code (700k+ lines :)) look like this after 10 years.
And as code convention page says, when uncertain ask, so thanks Marin and
Hristo for bringing it up.

Cheers
damencho

--
Regards,
Hristo.

Marin_Dzhigarov1 · 2013-09-30 08:03:13 UTC

Hello,

- Added support for FTP
- Added accessors to
net.java.sip.communicator.impl.gui.main.chat.ShowPreviewDialog's
maps.

Regards,
Marin

image preview.patch (52 KB)

···

On Fri, Sep 27, 2013 at 12:40 PM, Hristo Terezov <hristo@jitsi.org> wrote:

Hello,

I talked with Emil about the ftp use case that we can check the file type
by the file's extension. We can replace only the files that have image
extension.

On Fri, Sep 27, 2013 at 10:06 AM, Damian Minkov <damencho@jitsi.org>wrote:

Hi,

On Fri, Sep 27, 2013 at 9:37 AM, Marin Dzhigarov <marin@bluejimp.com>wrote:

> 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?

The practical example: one day some check need to be added when accessing msgIDToChatString,
but there is no get method, and by that time 3 more developers have used
the class and for a simple check a lot of code needs to be changed and a
lot of checking to make sure it is not used in other places ...
It is a good practice to have accessors, and that's how all of the code
is done. And we need to keep code consistent that is the reason all of the
code (700k+ lines :)) look like this after 10 years.
And as code convention page says, when uncertain ask, so thanks Marin and
Hristo for bringing it up.

Cheers
damencho

--
Regards,
Hristo.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

Hristo_Terezov · 2013-09-30 13:41:57 UTC

Hello,

I reviewed the patch and committed it.

···

On Mon, Sep 30, 2013 at 11:03 AM, Marin Dzhigarov <marin@bluejimp.com>wrote:

Hello,

- Added support for FTP
- Added accessors to net.java.sip.communicator.impl.gui.main.chat.ShowPreviewDialog's
maps.

Regards,
Marin

On Fri, Sep 27, 2013 at 12:40 PM, Hristo Terezov <hristo@jitsi.org> wrote:

Hello,

I talked with Emil about the ftp use case that we can check the file type
by the file's extension. We can replace only the files that have image
extension.

On Fri, Sep 27, 2013 at 10:06 AM, Damian Minkov <damencho@jitsi.org>wrote:

Hi,

On Fri, Sep 27, 2013 at 9:37 AM, Marin Dzhigarov <marin@bluejimp.com>wrote:

> 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?

The practical example: one day some check need to be added when
accessing msgIDToChatString, but there is no get method, and by that
time 3 more developers have used the class and for a simple check a lot of
code needs to be changed and a lot of checking to make sure it is not used
in other places ...
It is a good practice to have accessors, and that's how all of the code
is done. And we need to keep code consistent that is the reason all of the
code (700k+ lines :)) look like this after 10 years.
And as code convention page says, when uncertain ask, so thanks Marin
and Hristo for bringing it up.

Cheers
damencho

--
Regards,
Hristo.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

--
Regards,
Hristo.