Reordered #includes. Fixed --geometry flag. It is --geometry in man page but -geometry in usage and only -geometry is recognized by program. Simplified getCmdOption. Removed State enum, it was overkill. It is also possible to remove support for mandatory arguments (opt_argc < 0) as it is not used, but it does not add LOC so I left it there. Removed check for about:blank as start_page, it is not a special case.
I have split patch into three patches. geometry.diff fixes --geometry flag
Fixed --geometry flag. It is --geometry in man page but -geometry in usage and only -geometry is recognized by program.
getcmdoption.diff simplifies getCmdOption
Simplified getCmdOption. Removed State enum, it was overkill. It is also possible to remove support for mandatory arguments (opt_argc < 0) as it is not used, but it does not add LOC so I left it there.
aboutblank.diff removes unnecessary check.
Removed check for about:blank as start_page, it is not a special case.
Minor fixes (#include order, indentation) are not included.
Hi, On Sun, Jun 10, 2012 at 10:39:21PM +0400, 123 wrote:
I have split patch into three patches.
Good!
geometry.diff fixes --geometry flag
Fixed --geometry flag. It is --geometry in man page but -geometry in usage and only -geometry is recognized by program.
I'm not sure here. -geometry seems to be somewhat standard [1] and I don't want to break people's scripts etc without very good reason. So for now I adjusted the man page.
getcmdoption.diff simplifies getCmdOption
Simplified getCmdOption. Removed State enum, it was overkill. It is also possible to remove support for mandatory arguments (opt_argc < 0) as it is not used, but it does not add LOC so I left it there.
Hm, if the patch would fix some bug - even some minor stuff - I would commit it together with the cleanup, but not just for the reduced LOC. Any other takers?
aboutblank.diff removes unnecessary check.
Removed check for about:blank as start_page, it is not a special case.
Committed.
Minor fixes (#include order, indentation) are not included.
Good. Cheers, Johannes [1] http://www.progsoc.org/tfm/tfm97/node32.html
On Tue, Jun 12, 2012 at 10:26:45PM +0200, Johannes Hofmann wrote:
geometry.diff fixes --geometry flag
Fixed --geometry flag. It is --geometry in man page but -geometry in usage and only -geometry is recognized by program.
I'm not sure here. -geometry seems to be somewhat standard [1] and I don't want to break people's scripts etc without very good reason. So for now I adjusted the man page.
It is standard for traditional X applications (xclock, xterm etc.) to have only "long" options that start with one dash together with corresponding resource. Standard X tools mostly use XrmParseCommand [1] and you can see examples of options that start with one dash in Xlib documentation. I would even remove short options (like -v) and replace them with standard long options such as -version, but I guess you don't want to break compatibility for it.
getcmdoption.diff simplifies getCmdOption
Simplified getCmdOption. Removed State enum, it was overkill. It is also possible to remove support for mandatory arguments (opt_argc < 0) as it is not used, but it does not add LOC so I left it there.
Hm, if the patch would fix some bug - even some minor stuff - I would commit it together with the cleanup, but not just for the reduced LOC. Any other takers?
Now it looks like someone started writing a state machine for option parsing but then created a separate function for parsing just one option. Or maybe it is copied from legacy code. I will keep it until I find some related bug then, but IMO it is not good to mix cleanup with bugfixes (and it is not good to mix different patches in one in general). [1] http://www.tronche.com/gui/x/xlib/resource-manager/XrmParseCommand.html
On Thu, Jun 14, 2012 at 07:00:17PM +0000, corvid wrote:
Johannes wrote:
On Sun, Jun 10, 2012 at 10:39:21PM +0400, 123 wrote:
aboutblank.diff removes unnecessary check.
Removed check for about:blank as start_page, it is not a special case.
Committed.
about:blank no longer gives focus to Location.
ah, the details... I reverted it.
On Thu, Jun 14, 2012 at 07:00:17PM +0000, corvid wrote:
Johannes wrote:
On Sun, Jun 10, 2012 at 10:39:21PM +0400, 123 wrote:
aboutblank.diff removes unnecessary check.
Removed check for about:blank as start_page, it is not a special case.
Committed.
about:blank no longer gives focus to Location.
Right, it is because a_UIcmd_open_url works differently for NULL and about:blank. And it is commented in *identical* static function UIcmd_open_url_nbw. I overlooked this difference. If you change it back, add comment please.
On Thu, Jun 14, 2012 at 11:37:46PM +0400, 123 wrote:
On Thu, Jun 14, 2012 at 07:00:17PM +0000, corvid wrote:
Johannes wrote:
On Sun, Jun 10, 2012 at 10:39:21PM +0400, 123 wrote:
aboutblank.diff removes unnecessary check.
Removed check for about:blank as start_page, it is not a special case.
Committed.
about:blank no longer gives focus to Location.
Right, it is because a_UIcmd_open_url works differently for NULL and about:blank. And it is commented in *identical* static function UIcmd_open_url_nbw.
I overlooked this difference. If you change it back, add comment please.
done. Cheers, Johannes
a_UIcmd_open_url and UIcmd_open_url_nbw are identical. Difference is in argument name and comment. Why not remove UIcmd_open_url_nbw and replace it with a_UIcmd_open_url?
On Sat, Jun 16, 2012 at 02:49:47AM +0400, 123 wrote:
a_UIcmd_open_url and UIcmd_open_url_nbw are identical. Difference is in argument name and comment.
Why not remove UIcmd_open_url_nbw and replace it with a_UIcmd_open_url?
For simplicity. It is easier to differentiate the multiple situations: open in a new tab, in a new window, new bw (abstract). As a general rule we're not trying to optimize for size as the most important criteria these days; simplicity has served us better over the years. -- Cheers Jorge.-
participants (4)
-
corvid＠lavabit.com -
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
p37sitdu＠lavabit.com