[jitsi-dev] [jitsi] Call info frame equivalent for signaling in account info (#26)


#1

Adds an additional tab in the Account Info frame with an editor pane in which any additional information can be displayed. This can be seen as the equivalent of the call info frame but for chat/signaling information.
This patch also adds information to the pane with the remote server address and TLS information (if used), including a link to view the certificate.
The pane offers an easy way for anybody to add additional information before finding a more permanent first-class place in the GUI or to use it for more advanced information rarely needed by normal users etc.

(Replaces the previous pull request with same title as I screwed up this Git thing again...)
You can merge this Pull Request by running:

  git pull https://github.com/netmackan/jitsi tls-account-info-2

Or you can view, comment on it, or merge it online at:

  https://github.com/jitsi/jitsi/pull/26

-- Commit Summary --

  * Add an additional tab in the Account Info frame for showing any other information in a similar way as is currently possible in chat information frame. Note: The ViewCertificateFrame is duplicated from the call package so some refactoring is needed.
  * Clean up uncommented code.
  * Moved ViewCertificateFrame to dekstoputil as it will be used from multiple places
  * Removing duplicated ViewCertificateFrame
  * Add I18N strings.
  * Removed duplicated string.
  * Removing duplicated class.

-- File Changes --

    M resources/languages/resources.properties (1)
    M src/net/java/sip/communicator/plugin/accountinfo/AccountDetailsPanel.java (226)
    M src/net/java/sip/communicator/plugin/accountinfo/AccountInfoActivator.java (4)
    R src/net/java/sip/communicator/plugin/desktoputil/ViewCertificateFrame.java (3)

-- Patch Links --

https://github.com/jitsi/jitsi/pull/26.patch
https://github.com/jitsi/jitsi/pull/26.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26


#2

- for (Component component : valuesPanel.getComponents())
- component.setEnabled(false);
- if (aboutMeArea != null)
- aboutMeArea.setEnabled(false);
- applyButton.setEnabled(false);

Please don't change indentation when there's no real change to code.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11795875


#3

- for (Component component : valuesPanel.getComponents())
- component.setEnabled(false);
- if (aboutMeArea != null)
- aboutMeArea.setEnabled(false);
- applyButton.setEnabled(false);
+ TransparentPanel contactPanel = new TransparentPanel(new BorderLayout(10, 10));

Line length

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11795886


#4

+ stringBuffer.append(
+ "<html><body><p align=\"left\">"
+ + "<font color=\"" + fontColor + "\" size=\"3\">");
+
+ // Protocol name
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PROTOCOL"), protocolProvider.getProtocolName()));
+
+ // Server address and port
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.ADDRESS"),
+ protocolProvider.getAccountID().getServerAddress()));
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PORT"),
+ protocolProvider.getAccountID().getServerPort()));
+

Simply showing the values from the account config doesn't make much sense here. If I got your idea right, you want to show the details of the actual connection. So for my SIP account (which has a NAPTR and SRV record), it shows "Domain: example.com" and "Port: 5060", which is wrong (should be sip.example.com on port 5061). Same would go for XMPP if the server is on a non-standard port (which would easily work as the SRV indicates it) and you'd be interested in the actual server name and not the domain name.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11795954


#5

@@ -1221,4 +1289,106 @@ else if (oldDetail == null)
             }
         }
     }
+

These are not that important, but please avoid lines with just white-spaces.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11795963


#6

The layout of that whole thing is a bit awkward for this kind of dialog, but given that one of the GSoC projects will be around building a whole new UI I doubt it's worth spending to much on it. I think the information is useful, so I'd rather go with this layout than nothing.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26#issuecomment-40882317


#7

+ stringBuffer.append(
+ "<html><body><p align=\"left\">"
+ + "<font color=\"" + fontColor + "\" size=\"3\">");
+
+ // Protocol name
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PROTOCOL"), protocolProvider.getProtocolName()));
+
+ // Server address and port
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.ADDRESS"),
+ protocolProvider.getAccountID().getServerAddress()));
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PORT"),
+ protocolProvider.getAccountID().getServerPort()));
+

Yes, I think you are right. What I want is the actual address and port used.
Do you know where this information can be obtained?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11797311


#8

Regarding the awkward dialog, I agree with you. However, personally I think that a lot of things is problematic with the current GUI and with this change I just aimed for an easy integration with minimal change.

Creating a new GUI is something I welcome very much. I'm looking forward to see some progress in this area.

Some problems I see with the current GUI:
- Impossible to use without a mouse, ie not possible to switch focus between components, tabs etc
- Menu bars are not complete, much functionality is only accessible by clicking on toolbar buttons or right clicking on the contacts etc
- Why all this non-standard SIPComm*-components? Would have been better to mostly use the standard components and then the platform Look & Feel could have been used.
- Inconsistent margins around components, button placements and sizes of dialogs/frame. A proper GUI editor suggests the recommended margins and component placements automagically.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26#issuecomment-40891180


#9

@@ -1221,4 +1289,106 @@ else if (oldDetail == null)
             }
         }
     }
+

Right. I now enabled this in NetBeans IDE:
Tools -> Options -> Fonts & Colors -> Higlighting -> Trailing Whitespace
So hopefully I will spot those more easily in the future :slight_smile:

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11797351


#10

+ stringBuffer.append(
+ "<html><body><p align=\"left\">"
+ + "<font color=\"" + fontColor + "\" size=\"3\">");
+
+ // Protocol name
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PROTOCOL"), protocolProvider.getProtocolName()));
+
+ // Server address and port
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.ADDRESS"),
+ protocolProvider.getAccountID().getServerAddress()));
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PORT"),
+ protocolProvider.getAccountID().getServerPort()));
+

From the active socket, so for XMPP, through the same way you obtain the TLS information. For SIP, same complicated story as a couple of months ago.

Maybe a new OpSetConnectionInfo would make sense? The OpSetTls could then be derived from it and the isSecure() method (or similar) on the ProtocolProviderService could be moved there.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11797515


#11

Well, Jitsi is now 11 years old and most of the SIPComm* components to something to "style" the UI. Years back, the Swing UI didn't adapt to the OS styles as well as it does today, and even today it does it very lousy. Lets just blame most of it to Swing and see what we can achieve with CEF and HTML5.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26#issuecomment-40892318


#12

- this.add(buttonPanel);
+ this.add(buttonPanel);

There's more of these unnecessary changes, please review the whole method.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11797544


#13

@@ -207,30 +229,52 @@ public AccountDetailsPanel(AccountInfoDialog dialog,
         this.protocolProvider = protocolProvider;
         this.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));

+ if (OSUtils.IS_MAC)
+ {
+ fontColor = "FFFFFF";
+ }
+ else
+ {
+ fontColor = "000000";
+ }
+

Oh and this font color, do you have a Mac to test this? If I remember it right, then the call info panel is kind of a chromeless hover window on Mac (with black background), while it is a standard window on other OSs. As the text is inside a normal dialog here, I don't think the color change is necessary.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11797547


#14

+ stringBuffer.append(
+ "<html><body><p align=\"left\">"
+ + "<font color=\"" + fontColor + "\" size=\"3\">");
+
+ // Protocol name
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PROTOCOL"), protocolProvider.getProtocolName()));
+
+ // Server address and port
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.ADDRESS"),
+ protocolProvider.getAccountID().getServerAddress()));
+ stringBuffer.append(getLineString(R.getI18NString(
+ "service.gui.PORT"),
+ protocolProvider.getAccountID().getServerPort()));
+

Before:
![accountinfo-before](https://cloud.githubusercontent.com/assets/48208/2751238/70a14bb0-c8bc-11e3-8f6b-f9a167945fb4.png)

After (first tab):
![accountinfo-after-1](https://cloud.githubusercontent.com/assets/48208/2751239/802211be-c8bc-11e3-9a4b-e3afead628d0.png)

After (second tab):
![accountinfo-after-2](https://cloud.githubusercontent.com/assets/48208/2751240/87c16ee2-c8bc-11e3-9a56-bb096cbd3d32.png)

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11798860


#15

@@ -207,30 +229,52 @@ public AccountDetailsPanel(AccountInfoDialog dialog,
         this.protocolProvider = protocolProvider;
         this.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));

+ if (OSUtils.IS_MAC)
+ {
+ fontColor = "FFFFFF";
+ }
+ else
+ {
+ fontColor = "000000";
+ }
+

No you are right this was unnecessary. Didn't realize the CallInfoFrame wasn't a Frame... :slight_smile:

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r11798934


#16

Regarding the OpSetConnectionInfo:
I have implemented this and obtain the hostname from the socket. However, I am not sure it works exactly as expected as it seems it does a reverse lookup to get the hostname instead of using the actual hostname... Maybe te need to store the actual hostname used in ProtocolProviderServiceJabberImpl instead of later on taking it from the socket.

Also for the OpSetConnectionInfo, I only added the getServerAddress and getServerPort methods, however I feel it is likely that other information might be needed in the future. Would that be possible to add in the interface later on or how is that normally handled?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26#issuecomment-40903682


#17

Ups, some comments went directly to the commit in your repo. Regading the UI: giving the two tabs a border and making the "Other" tab with the connection info a very simple JTable (or a 2-column GridLayout with JLabels) would make it way less awkward.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26#issuecomment-40904897


#18

- The comments directly in the commit has been addressed now.
- Border around the tab, I agree, however, and this was why I raised the issue about the other parts of the GUI: This is general issue for many of the usages of tab panes in Jitsi. I can fix it here however, I would suggest creating a specific issue for that and fix it in all places to get a more consistent look.
- My idea was to create an equivalent of call info frame with a simple editor pane to have an easy way of adding any useful information before finding a more permanent place in the GUI. If you don't like this concept I would rather try to design a proper panel for the connection and TLS information. I would then rename the tab from "Other" to "Connection".

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26#issuecomment-42037641


#19

@@ -0,0 +1,24 @@
+/*
+ * Jitsi, the OpenSource Java VoIP and Instant Messaging client.
+ *
+ * Distributable under LGPL license.
+ * See terms of license at gnu.org.
+ */
+package net.java.sip.communicator.service.protocol;
+
+import java.net.InetSocketAddress;

This should be a *-import.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r12231257


#20

+ /**
+ * Invoked when user clicks a link in the editor pane.
+ * @param e the event
+ */
+ public void hyperlinkUpdate(HyperlinkEvent e)
+ {
+ // Handle "View certificate" link
+ if (e.getEventType() == HyperlinkEvent.EventType.ACTIVATED
+ && CERTIFICATE_URL.equals(e.getDescription()))
+ {
+ Certificate[] chain = protocolProvider
+ .getOperationSet(OperationSetTLS.class)
+ .getServerCertificates();
+
+ ViewCertificateFrame certFrame =
+ new ViewCertificateFrame(chain, null,

I think there's an indent-space missing in the lines above (or one too much below, not sure from the online view).

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi/pull/26/files#r12231293