Relay panel could benefit from more optional columns

added component::archived/vidalia owner::sirop priority::low resolution::fixed status::closed type::enhancement labels

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.

Trac:
Username: beduin

I am sorry, correct branch name is tick!#2731 (closed)

Trac:
Username: beduin

Trac:
Username: sirop
Owner: chiiph to sirop
Status: new to assigned

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.

Trac:
Username: sirop
Status: assigned to needs_review

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.

Trac:
Status: needs_review to needs_revision

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:

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

Trac:
Username: sirop

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.

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);
}

Trac:
Username: sirop
Status: needs_revision to needs_review

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?

Trac:
Status: needs_review to needs_revision

Trac:
Username: sirop

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.

Trac:
Username: sirop
Status: needs_revision to needs_review

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".

Trac:
Status: needs_review to needs_revision

Trac:
Username: sirop

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

loadSettings() loads only Relay options as (re)loading Splitter options would unset the
current (but not yet saved) splitter geometry.
NetworkMap5.png shows the overall picture. The Settings frame does resize, the GroupBox and the buttons inside the frame do not.

Trac:
Username: sirop
Status: needs_revision to needs_review

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!

Trac:
Status: needs_review to needs_revision

Trac:
Username: sirop

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

Followed your advice -- deleted redundant connect() calls ans slots, attached changes to user's clicking the Save button.
NetworkMap6.png shows the new layout, GridLayout for the GroupBox.

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

line 177 had double white space before LF.

Trac:
Username: sirop
Status: needs_revision to needs_review

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

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

Relay panel could benefit from more optional columns

Child items 0

Activity