Opened 8 years ago

Closed 7 years ago

#2731 closed enhancement (fixed)

Relay panel could benefit from more optional columns

Reported by: chiiph Owned by: sirop
Priority: Low Milestone:
Component: Archived/Vidalia Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Migrated from old track:
"Node footprint, IP address, uptime, bandwidth, platform etc could be optionally displayed as columns. Being able to sort by various criteria would be useful. "

Child Tickets

TicketTypeStatusOwnerSummary
#5627defectclosedchiiphTwo tabs instead of white spaces in NetViewer.cpp

Attachments (6)

RelayColumns1.png (8.8 KB) - added by sirop 7 years ago.
RelayColumns2.png (9.7 KB) - added by sirop 7 years ago.
NetworkMapSettings.png (12.1 KB) - added by sirop 7 years ago.
NetworkMapSettings2.png (10.5 KB) - added by sirop 7 years ago.
NetworkMap5.png (253.8 KB) - added by sirop 7 years ago.
NetworkMap6.png (12.0 KB) - added by sirop 7 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by beduin

Hi, all.

I have done some work on this ticket. I am looking forward to see your opinion about made changes.

You can find my work on https://github.com/beduin/vidalia. You need branch tick#237. Description of what I was going to do, what I have done and what problems remains you can read in README.taskDescription file in the repo root.

As upstream repo I am using https://github.com/mwenge/vidalia.

comment:2 Changed 8 years ago by beduin

I am sorry, correct branch name is tick#2731

comment:3 Changed 7 years ago by sirop

Owner: changed from chiiph to sirop
Status: newassigned

comment:4 Changed 7 years ago by sirop

Status: assignedneeds_review

https://github.com/sirop/vidalia_apha/commits/ticket2731_RelayPanel

I did not add node footprint, uptime and platform in order not to overload the Relay Panel.
Footprint and platform are anyway shown at the tool tip.

This commit is applied to
git clone -b alpha git://git.torproject.org/chiiph/vidalia.git.

comment:5 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

The only comment I have is that I see some bad indentation. Other than that it looks good, but where's the optional part?

You might want to mimic the Bandwidth graph's configuration for this.

comment:6 Changed 7 years ago by sirop

https://github.com/sirop/vidalia_alpha/commit/2440dabc87a177aaabe0147a2c7e3141a495c8cc

Now the optional part is there. I made a TabWidget, as button like in the BW graph would not look so nice in the small Relay panel.

Self-critique:

  1. Is the observed and (not the minimum) bandwidth a correct choice?
  2. Is the uptime, even if adjusted, correct choice as the relay panel is updated only once an hour?
  3. Indentation?

comment:7 Changed 7 years ago by sirop

Status: needs_revisionneeds_review

https://github.com/sirop/vidalia_0.3.2_alpha/commit/a02b347af69b0322972b4f0694b75236bfd2e3c1


                 tb->setToolButtonStyle(Qt::ToolButtonTextBesideIcon);
 176 	 214 	   ui.horizontalLayout->addWidget(tb);
 177 	   	-  
   	 215 	+
 178 	 216 	   /* Restore the state of each splitter */

This new line is added/removed due to double white space that was there before.

  1. Sorting IPs:
/** Convert IP numbers to quint64 for the comparison operator 
 * filling the dot separated groups with zeroes if necessary.
 */
quint64
RouterListItem::iptoquint64(const RouterListItem *ListItem) const
{
  bool ok;
  quint64 a_IPnumber;
  QString a_IPString;
  a_IPString = ListItem->descriptor().ip().toString();
  QStringList ListIP = a_IPString.split(".", QString::SkipEmptyParts);
  a_IPString = "";
  for (int i = ListIP.size()-1; i >= 1; i--) {
  ListIP[i] = ListIP[i].rightJustified(3, '0');
  a_IPString.prepend(ListIP[i]);
  } 
  a_IPString.prepend(ListIP[0]);
  a_IPnumber = a_IPString.toULongLong(&ok, 10);
  return(a_IPnumber);
}

comment:8 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

Important issues:

  • You should mimic what classes like BandwidthGraph do for settings (use getSettings, saveSettings)
  • (This point is more of a general programming issue, you shouldn't have to take into account this if you follow the previous point) If you are going to create VSettings *settings and delete it a little bit after that, it's faster to just use VSettings settings. Although it might be better to have a class global VSettings *_settings.
  • You seem to have changed the RouterListWidget for a QTabWidget, you should change that back.
  • You used " Relay Panel " as a string, you should use spaces to format something in the GUI because it will change if the user has a different font size configuration. Please use spacers for that, or something similar.

A couple of nitpicks:

  • The changes file should be 60 columns wrapped
  • Things like:
ui.treeRouterList->setColumnHidden(RouterListWidget::IPnumberColumn,
  !(settings->value(SETTING_IP_COLUMN, DEFAULT_IP_COLUMN).toBool()));

Should be indented to something like:

ui.treeRouterList->setColumnHidden(RouterListWidget::IPnumberColumn,
                                   !(settings->value(SETTING_IP_COLUMN, 
                                                     DEFAULT_IP_COLUMN)
                                                    .toBool()));
  • Nesting code should be indented properly too.
  • Variables named a_IPnumber should be ipNumber, please don't mix camelCase with other naming styles.

I won't apply your patch and test until these other issues are solved,
so I will ask you: do you hide the settings like the Bandwidth Graph
or the Message Log does?

Changed 7 years ago by sirop

Attachment: RelayColumns1.png added

Changed 7 years ago by sirop

Attachment: RelayColumns2.png added

comment:9 Changed 7 years ago by sirop

Status: needs_revisionneeds_review

https://github.com/sirop/vidalia_0.3.2_alpha/commit/a3e9ebb475de6b1028386e52ccebe6ea92f24dc0

I have also attached two *.png's showing the Relay panel as a TabWidget with TreeListWidget inside the first tab.
BW Graph is a Vidalia tab, whereas Relay panel is only a panel in the Network Tab
and thus much smaller. That's why I thought that a extra button on the Relay panel
would be not appropriate.

comment:10 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

Code looks good.

The GUI layout on the other hand needs a little bit more work. I suggest you mimic how tabs like Message Log handle settings. I think it may be a good idea to have a settings button in the Network Map's toolbar, that makes a separate groupbox show at the bottom. In the future, if we add more options to configure the map, it will be a good place to put other configuration UI.

I agree that this part of Vidalia may have a lot of widgets, but I can't think a better way of doing this. Vidalia should have a regular GUI across all its sections, so users that got used to how things are done with the "old parts" of it understand the "new parts".

Changed 7 years ago by sirop

Attachment: NetworkMapSettings.png added

Changed 7 years ago by sirop

Attachment: NetworkMapSettings2.png added

Changed 7 years ago by sirop

Attachment: NetworkMap5.png added

comment:11 Changed 7 years ago by sirop

Status: needs_revisionneeds_review

https://github.com/sirop/vidalia_0.3.2_alpha/commit/34d73cecc05ba7d137428b465cbcc433e2bbc359

  1. loadSettings() loads only Relay options as (re)loading Splitter options would unset the

current (but not yet saved) splitter geometry.

  1. NetworkMap5.png shows the overall picture. The Settings frame does resize,

the GroupBox and the buttons inside the frame do not.

comment:12 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

It's looking better :)

Two more things:

  • Why are you using Qt's special naming convention to have slots without the need of connect() calls, and connecting them too? It seems redundant. Also, you should use camelCase names for methods (with normal names), and connect them as you are doing already. For example, on_chkShowUptime_clicked should be something like chkShowUptimeClicked. *But*, with the new loadSettings/saveSettings behavior, is there are real need for those slots? Imagine this: what would a user think if she clicks on a checkbox and suddenly a column appears/disappears without having to press "Save"? It's kind of confusing, don't you think?
  • Regarding the Layout, the Save and Cancel buttons should be stitched to the right of the dialog. It's probably a layout issue, if you have, say, a GridLayout for the whole settings groupbox, it will stretch as needed and you won't have buttons like in your screenshot.

Keep it up and we will be merging this in no time!

Changed 7 years ago by sirop

Attachment: NetworkMap6.png added

comment:13 Changed 7 years ago by sirop

Status: needs_revisionneeds_review

https://github.com/sirop/vidalia_0.3.2_alpha/commit/f4bbf3f44f061049b3d435709638bc1477adcd2e

  1. Followed your advice -- deleted redundant connect() calls ans slots, attached changes

to user's clicking the Save button.

  1. NetworkMap6.png shows the new layout, GridLayout for the GroupBox.

3.

                ui.horizontalLayout->addWidget(tb);
177 	   	-  
  	 197 	+
178 	 198 	   /* Restore the state of each splitter */

line 177 had double white space before LF.

comment:14 Changed 7 years ago by chiiph

Resolution: fixed
Status: needs_reviewclosed

Merged. Sorry for the delay! It will be out with 0.3.3-alpha.

Note: See TracTickets for help on using tickets.