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

Re: [tor-bugs] #2163 [Vidalia]: Disable menubar icon in OS X

#2163: Disable menubar icon in OS X
---------------------------------+------------------------------------------
 Reporter:  jabes                |          Owner:  chiiph        
     Type:  enhancement          |         Status:  assigned      
 Priority:  normal               |      Milestone:                
Component:  Vidalia              |        Version:  Vidalia 0.2.10
 Keywords:  os x, menubar, icon  |         Parent:                
   Points:                       |   Actualpoints:                
---------------------------------+------------------------------------------

Comment(by edmanm):

 I like the idea behind the patch, but would recommend not merging it as
 is. Here's a few suggestions for improvement:

 1) You're trying to specify a `.plist` file, but you're pointing to the
 unconfigured one (i.e., the `.plist` that hasn't had the proper `${foo}$`
 replacements filled in:

 {{{
  ## Specify location of custom Info.plist file
 set_target_properties(${vidalia_BIN} PROPERTIES MACOSX_BUNDLE_INFO_PLIST
 ../pkg/osx/MacOSXBundleInfo.plist.in)
 }}}

 You're missing a `configure_file()` call, that should configure the
 `.plist.in` and place the result in the appropriate location under
 `${CMAKE_CURRENT_BINARY_DIR}`. See `src/vidalia/CMakeLists.txt` for some
 examples.

 2) These string constants scattered across several source files are bad
 practice:

 {{{
 ui.rdoIconPrefBoth->setChecked(_settings->getIconPref() == "Both");
 ui.rdoIconPrefTray->setChecked(_settings->getIconPref() == "Tray");
 ui.rdoIconPrefDock->setChecked(_settings->getIconPref() == "Dock");
 }}}

 See `TorSettings::getAuthenticationMethod()` and
 `TorSettings::setAuthenticationMethod()` for an example of a cleaner, less
 error-prone way to interface with config file settings like that.

 3) The name for the setting could be more descriptive:

 {{{
 #if defined(Q_WS_MAC)
 #define SETTING_ICON_PREF       "IconPref"
 #endif
 }}}

 Perhaps something like `IconDisplayPreference`, `DockIconPreference` or
 something similar would be more descriptive. If in doubt, opt for a
 clearer name rather than trying to save typing a few characters.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2163#comment:4>
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