Here is a list of problems with the patch:
- Way too many options have been added, making help harder to understand
(for users and developers).
- Remove anything used for debugging and avoid sending it to us when we are
reviewing the patch.
- The options have too many abbreviated forms (e.g. see below). Single dash
should be for one letter short options (e.g. -nc is bad), and abbreviated
long options should be a strict subset from the start of the long option
(e.g. --nc is not a substring of --no-color)
> case -nc --nc --no-c --no-co --no-col --no-colo --no-color
- isatty should be used to detect if colour should be used. This allows
(help | cat) to be used to avoid using colour.
- The case statement to check for fish commands (see below) could be
replaced with a simple condition (if test
-f $__fish_datadir/man/$fish_help_item.1) in the "*" case.
> case (ls $__fish_datadir/man | sed '/\.1$/ s/\.1$//')
- You make translations impossible by not using gettext, e.g. printf (_ '%s:
Could not find a web browser.\n') help, and instead using echo with
expansion which can't easily be translated.
> echo "$cmd_color set --global $args[1] $args[2]$normal"
- You should use Grissoms recent fish_indent patches if you are going to use
fish_indent:
http://gitorious.org/~grissiom/fish-shell/grissioms-fish-shell/commits/master
but an even better idea is to not touch lines that don't need changing, and
follow the existing code style. That way the diff with the old version is
shorter, so we can review that instead of the whole file.
- Use the return code from (man -w $fish_help_item) instead of using grep,
and don't output an error like message when a man page is found.
> if man -w $fish_help_item | grep -q "^/"
> echo "No HTML help available for \"$fish_help_item\" but but there is a"
> echo "system man page. Showing manual page via \"man $fish_help_item\"
..."
> man $fish_help_item
> return
> end
Are we trying to discourage people from using help for system man pages? You
also have a verbose error when no man page is found that documents the lack
of command completion for system man pages. Users can get a list of help
topics with (help), and *should* be able to get a list of commands using
(help commands), since the table of contents for those pages should provide
that list.
- Your message for update-alternatives is incorrect:
> echo "$cmd_color update-alternatives ..config $fish_browser$normal"
You replaced the dashes with periods for the config option.
- Your verbose messages tell the user to use (set --global) instead of (set
--universal) for changing the settings for their default browser.
- Not only is your help function much more complicated, but you also seem to
be exposing that complexity by default through the messages even when no
error occurs. I don't know if this is the way to go since the fish user
documentation states that "Configurability is the root of all evil". Maybe
we should focus on better default behaviour than to add more configuration
options.
For instance, when the user sets the default browser through chrome,
firefox, or gnome-default-applications-properties (i.e.
System->Preferences->Preferred Applications), there is a common gconf
setting that is set at "/desktop/gnome/url-handlers/http/command". This is
gnome specific, but KDE probably has something similar. We can use the
presence of GNOME_DESKTOP_SESSION_ID to detect if gnome is being used, and
we can also check for the gconftool-2 command, then we can use (gconftool-2
--get /desktop/gnome/url-handlers/http/command) to get the command.
I looked for a desktop agnostic solution by looking at freedesktop.org. The
shared mime info seemed promising, because you could use (xdg-mime query
default text/html) to get the command to open html files which I figured
would open their default browser. Unfortunately I found the options I
mentioned above for setting the default browser didn't affect the browser
found through xdg-mime, even though this could be set though a file in the
user's home directory ~/.local/share/applications/mimeapps.list
I mentioned that update-alternatives can be used to change where
/usr/bin/{,x-}www-browser points, but this is a system wide settings so also
requires sudo privileges.
------------------------------------------------------------------------------
What happens now with your Lotus Notes apps - do you make another costly
upgrade, or settle for being marooned without product support? Time to move
off Lotus Notes and onto the cloud with Force.com, apps are easier to build,
use, and manage than apps on traditional platforms. Sign up for the Lotus
Notes Migration Kit to learn more. http://p.sf.net/sfu/salesforce-d2d
_______________________________________________
Fish-users mailing list
Fish-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/fish-users
