[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #2731 [Vidalia]: Relay panel could benefit from more optional columns

#2731: Relay panel could benefit from more optional columns
-------------------------+--------------------------------------------------
 Reporter:  chiiph       |          Owner:  sirop         
     Type:  enhancement  |         Status:  needs_revision
 Priority:  minor        |      Milestone:                
Component:  Vidalia      |        Version:                
 Keywords:               |         Parent:                
   Points:               |   Actualpoints:                
-------------------------+--------------------------------------------------
Changes (by chiiph):

  * status:  needs_review => needs_revision


Comment:

 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?

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2731#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs