Bug 2171
Hello All,
Please forgive my ignorance if I ask obvious questions - I'm just starting out with NetSurf!
Steve suggested I take a look at Bug 2171 (http://bugs.netsurf-browser.org/mantis/view.php?id=2171) as a way of getting to grips with the source code.
I've been looking and scratching my head for a day or so.
I get that the menu for the Iconbar is set up in iconbar.c with this struct:
static const struct ns_menu iconbar_definition = {
"NetSurf", {
{ "Info", NO_ACTION, &dialog_info },
{ "AppHelp", HELP_OPEN_CONTENTS, 0 },
{ "Open", BROWSER_NAVIGATE_URL, 0 },
{ "Open.OpenURL", BROWSER_NAVIGATE_URL, &dialog_openurl },
{ "Open.HotlistShow", HOTLIST_SHOW, 0 },
{ "Open.HistGlobal", HISTORY_SHOW_GLOBAL, 0 },
{ "Open.ShowCookies", COOKIES_SHOW, 0 },
{ "Choices", CHOICES_SHOW, 0 },
{ "Quit", APPLICATION_QUIT, 0 },
{NULL, 0, 0}
}
Which uses the enum in menus.h to set up the menu order and creates the menu according to functions in menu.c
As far as I can gather the actual text (i.e., "Help... F1") is set up using the function ro_gui_menu_transate
Is that correct? I can't seem to find any references to the text in the source code ("Help..." or any of the other strings). Am I missing something obvious?
Glen
Re: Bug 2171
On Thu, Mar 05, 2015 at 12:11:54PM +0100, Glen Walker wrote: > Hello All, Hi, Please avoid sending HTML mail to this list. B.
Re: Re: Bug 2171
Apologies again - I might have sent this out twice (first in HTML and then in
plaintext but I don't think it went to everyone).
This is the first time I have ever used a mailing list to talk about anything -
I'm far more used to forums...
But I digress...if you have received the HTML version I apologise and please
delete it. Here is the plaintext version:
Hello All,
Please forgive my ignorance if I ask obvious questions - I'm just starting out
with NetSurf!
Steve suggested I take a look at Bug 2171
(http://bugs.netsurf-browser.org/mantis/view.php?id=2171) as a way of getting
to grips with the source code.
I've been looking and scratching my head for a day or so.
I get that the menu for the Iconbar is set up in iconbar.c with this struct:
static const struct ns_menu iconbar_definition = {
"NetSurf", {
{ "Info", NO_ACTION, &dialog_info },
{ "AppHelp", HELP_OPEN_CONTENTS, 0 },
{ "Open", BROWSER_NAVIGATE_URL, 0 },
{ "Open.OpenURL", BROWSER_NAVIGATE_URL, &dialog_openurl },
{ "Open.HotlistShow", HOTLIST_SHOW, 0 },
{ "Open.HistGlobal", HISTORY_SHOW_GLOBAL, 0 },
{ "Open.ShowCookies", COOKIES_SHOW, 0 },
{ "Choices", CHOICES_SHOW, 0 },
{ "Quit", APPLICATION_QUIT, 0 },
{NULL, 0, 0}
}
Which uses the enum in menus.h to set up the menu order and creates the menu
according to functions in menu.c
As far as I can gather the actual text (i.e., "Help... F1") is set up using the
function ro_gui_menu_transate
Is that correct? I can't seem to find any references to the text in the source
code ("Help..." or any of the other strings). Am I missing something obvious?
Glen
Re: Re: Bug 2171
On Thu, Mar 05, 2015 at 12:38:07PM +0100, Glen Walker wrote:
> Which uses the enum in menus.h to set up the menu order and creates the menu
> according to functions in menu.c
>
> As far as I can gather the actual text (i.e., "Help... F1") is set up using
> the function ro_gui_menu_transate
>
> Is that correct? I can't seem to find any references to the text in the
> source code ("Help..." or any of the other strings). Am I missing something
> obvious?
The actual text used in the UI is looked up in the Messages file so
NetSurf (like many RISC OS applications) can be localised without a
recompile.
See resources/FatMessages (this file is split up during the build
process automatically.)
B.
Re: Re: Bug 2171
Aha! Excellent, thank you! Do you think it would be appropriate to define another version of "AppHelp" (and other menu entries) for use where the the function key shortcut does not apply? Something like this: en.ro.AppHelp:Help... F1 de.ro.AppHelp:Hilfe... F1 fr.ro.AppHelp:Aide... F1 it.ro.AppHelp:Aiuto... F1 nl.ro.AppHelp:Hulp... F1 en.ro.AppHelpNoFunc:Help... de.ro.AppHelpNoFunc:Hilfe... fr.ro.AppHelpNoFunc:Aide... it.ro.AppHelpNoFunc:Aiuto... nl.ro.AppHelpNoFunc:Hulp... Or is there a cleaner way? I'll get my thinking cap on... Glen
Re: Re: Bug 2171
On Thu, Mar 05, 2015 at 12:56:53PM +0100, Glen Walker wrote: > Aha! Excellent, thank you! > > Do you think it would be appropriate to define another version of "AppHelp" > (and other menu entries) for use where the the function key shortcut does not > apply? Something like this: > > en.ro.AppHelp:Help... F1 > de.ro.AppHelp:Hilfe... F1 > fr.ro.AppHelp:Aide... F1 > it.ro.AppHelp:Aiuto... F1 > nl.ro.AppHelp:Hulp... F1 > > en.ro.AppHelpNoFunc:Help... > de.ro.AppHelpNoFunc:Hilfe... > fr.ro.AppHelpNoFunc:Aide... > it.ro.AppHelpNoFunc:Aiuto... > nl.ro.AppHelpNoFunc:Hulp... > > Or is there a cleaner way? I'll get my thinking cap on... I suspect it's either this (although I would use NoAccel or NoShortcut), or an ugly heuristic to modify and truncate after ... - I think I prefer the former. Of course, we could have a system of user-definable shortcuts such that the menu entries are generated programmatically, but this is obviously a huge task. B.
Re: Re: Bug 2171
On Thu, 05 Mar 2015 12:01:05 -, Rob Kendrick wrote: On Thu, Mar 05, 2015 at 12:56:53PM +0100, Glen Walker wrote: Aha! Excellent, thank you! Do you think it would be appropriate to define another version of "AppHelp" (and other menu entries) for use where the the function key shortcut does not apply? Something like this: en.ro.AppHelp:Help... F1 de.ro.AppHelp:Hilfe... F1 fr.ro.AppHelp:Aide... F1 it.ro.AppHelp:Aiuto... F1 nl.ro.AppHelp:Hulp... F1 en.ro.AppHelpNoFunc:Help... de.ro.AppHelpNoFunc:Hilfe... fr.ro.AppHelpNoFunc:Aide... it.ro.AppHelpNoFunc:Aiuto... nl.ro.AppHelpNoFunc:Hulp... Or is there a cleaner way? I'll get my thinking cap on... I suspect it's either this (although I would use NoAccel or NoShortcut), or an ugly heuristic to modify and truncate after ... - I think I prefer the former. Of course, we could have a system of user-definable shortcuts such that the menu entries are generated programmatically, but this is obviously a huge task. B. Couldn't the messages in the UI messages file contain a special character to denote the start of a keyboard shortcut? Something that wouldn't make sense for a menu item, like a CR? I suppose that's not very friendly in terms of the messages file itself, so maybe something else, like a forward slash. The ro_gui_translate() (if that is the right function - I haven't looked) could be passed an extra parameter to either truncate the string at that point, or remove the special character from the string. Thoughts?
Re: Re: Bug 2171
On Thu, Mar 05, 2015 at 02:26:26PM -, WPB wrote: > Couldn't the messages in the UI messages file contain a special > character to denote the start of a keyboard shortcut? Something that > wouldn't make sense for a menu item, like a CR? I suppose that's not > very friendly in terms of the messages file itself, so maybe > something else, like a forward slash. > > The ro_gui_translate() (if that is the right function - I haven't > looked) could be passed an extra parameter to either truncate the > string at that point, or remove the special character from the > string. I think the strings are handed around as const char *; processing them would involve making copies in memory. B.
Re: Re: Bug 2171
On Thu, 05 Mar 2015 14:49:59 -, Rob Kendrick wrote: On Thu, Mar 05, 2015 at 02:26:26PM -, WPB wrote: Couldn't the messages in the UI messages file contain a special character to denote the start of a keyboard shortcut? Something that wouldn't make sense for a menu item, like a CR? I suppose that's not very friendly in terms of the messages file itself, so maybe something else, like a forward slash. The ro_gui_translate() (if that is the right function - I haven't looked) could be passed an extra parameter to either truncate the string at that point, or remove the special character from the string. I think the strings are handed around as const char *; processing them would involve making copies in memory. B. I wondered if that might be the case. Fair enough, then. Perhaps the duplicate messages option is cleanest without a large development overhead. Bit of a headache for internationalizing, but I think the FatMessages file uses a dot-separated hierarchical structure for key names, doesn't it? So you could have en.ro.AppHelp and en.ro.AppHelp.NoFunc, for example, to keep it clean, perhaps?
Re: Re: Bug 2171
On Thu, Mar 05, 2015 at 02:55:49PM -, WPB wrote: > I wondered if that might be the case. Fair enough, then. Perhaps the > duplicate messages option is cleanest without a large development > overhead. Bit of a headache for internationalizing, but I think the > FatMessages file uses a dot-separated hierarchical structure for key > names, doesn't it? So you could have en.ro.AppHelp and > en.ro.AppHelp.NoFunc, for example, to keep it clean, perhaps? Sure, after the first two dots it's just a string up to : that is used to key it, IIRC. But again, please not "NoFunc". B.
Re: Re: Bug 2171
> Sent: Thursday, March 05, 2015 at 2:49 PM
> From: "Rob Kendrick"
> To: netsurf-dev@netsurf-browser.org
> Subject: Re: Re: Bug 2171
>
> On Thu, Mar 05, 2015 at 02:26:26PM -, WPB wrote:
> > Couldn't the messages in the UI messages file contain a special
> > character to denote the start of a keyboard shortcut? Something that
> > wouldn't make sense for a menu item, like a CR? I suppose that's not
> > very friendly in terms of the messages file itself, so maybe
> > something else, like a forward slash.
> >
> > The ro_gui_translate() (if that is the right function - I haven't
> > looked) could be passed an extra parameter to either truncate the
> > string at that point, or remove the special character from the
> > string.
>
> I think the strings are handed around as const char *; processing them
> would involve making copies in memory.
>
> B.
>
>
This is basically how wxWidgets handles menu entries but to do so in NetSurf
would require quite a function to parse each string and a flag to determine if
the short-cut is actually required or not. Something like:
char * menu_create_entry(char *entry, bool shortcut)
{
if (!shortcut) {
parse entry char to find special modifier character
and return char with everything after that chopped off
} else {
parse entry char and return with just the modifier character
deleted so it is a plain text string
}
}
For reference here is an excerpt from the wxWidgets documentation
http://docs.wxwidgets.org/trunk/classwx_menu_item.html#a6e9b0e1b786fa84250a42c88d84aed2b
"virtual void wxMenuItem::SetItemLabel (const wxString & label)" sets the label
associated with the menu item.
The label string for the normal menu items (not separators) may include the
accelerator which can be used to activate the menu item from keyboard. An
accelerator key can be specified using the ampersand & character. In order to
embed an ampersand character in the menu item text, the ampersand must be
doubled.
Optionally you can specify also an accelerator string appending a tab character
\t followed by a valid key combination (e.g. CTRL+V). Its general syntax is any
combination of "CTRL", "RAWCTRL", "ALT" and "SHIFT" strings (case doesn't
matter) separated by either '-' or '+' characters and followed by the
accelerator itself. Notice that CTRL corresponds to the "Ctrl" key on most
platforms but not under Mac OS where it is mapped to "Cmd" key on Mac keyboard.
Usually this is exactly what you want in portable code but if you really need
to use the (rarely used for this purpose) "Ctrl" key even under Mac, you may
use RAWCTRL to prevent this mapping. Under the other platforms RAWCTRL is the
same as plain CTRL.
The accelerator may be any alphanumeric character, any function key (from F1 to
F12) or one of the special characters listed in the table below (again, case
doesn't matter):
DEL or DELETE: Delete key
BACK : Backspace key
INS or INSERT: Insert key
ENTER or RETURN: Enter key
PGUP: PageUp key
PGDN: PageDown key
LEFT: Left cursor arrow key
RIGHT: Right cursor arrow key
UP: Up cursor arrow key
DOWN: Down cursor arrow key
HOME: Home key
END: End key
SPACE: Space
TAB: Tab key
ESC or ESCAPE: Escape key (Windows only)
Re: Re: Bug 2171
> But again, please not "NoFunc". > > B. > > I agree! Sorry, that was me. NoAccellerator/NoAccel or NoShortcut are better. In fact I propose that if we adopt that strategy then "NoShortcut" is probably the most obvious one and should be used.
Re: libcss memory leak
On 03/03/15 16:55, Ralf Junker wrote: libcss may leak memory if css_stylesheet_data_done() > is not called. See the attached source for an example. Hi Ralf, thanks for pointing this out. I agree, the API does not make it clear that you must call _data_done before _destroy if you want to abort. I need to check that adding unref_interned_strings_in_tokens(parser); in css__parser_destroy does not introduce an attempt to unref token strings twice in the case where both _data_done and _destroy are called. I'll try to look into it soon. Cheers, -- Michael Drake http://www.smoothartist.com/
Re: Re: Bug 2171
On 5 Mar, Rob Kendrick wrote in message
<20150305150242.gt31...@platypus.pepperfish.net>:
> On Thu, Mar 05, 2015 at 02:55:49PM -, WPB wrote:
> > I wondered if that might be the case. Fair enough, then. Perhaps the
> > duplicate messages option is cleanest without a large development
> > overhead. Bit of a headache for internationalizing, but I think the
> > FatMessages file uses a dot-separated hierarchical structure for key
> > names, doesn't it? So you could have en.ro.AppHelp and
> > en.ro.AppHelp.NoFunc, for example, to keep it clean, perhaps?
>
> Sure, after the first two dots it's just a string up to : that is used to
> key it, IIRC.
Yes, but the RISC OS front-end's menu code uses . as a token separator in
the menu definition. I suspect (although having just spent a few minutes
looking at the remaining content of menus.c, I'm remembering why I left it
alone last time) that "AppHelp.NoFunc" could be treated as a menu and
submenu, where the parent entry/submenu title is from the "AppHelp" token
and the submeny entry is from "NoFunc".
As a hint to anyone else familiar with the RISC OS Wimp who's trying to
follow the code in menus.c/h, it looks as if what NetSurf calls a "menu"
(struct menu_definition) is the whole thing, submenus and all. The wimp
tends to refer to that as a "menu tree": a struct menu_definition could well
refer to many wimp_menu blocks. Once one realises that, it starts to make a
bit more sense; I think some of the comments could be a bit 'historic',
though.
> But again, please not "NoFunc".
There's no need for AppHelp, anyway.
The only thing to use AppHelp is the iconbar menu, so this token can be
changed directly to remove the F1. The main menu (defined in window.c as it
belongs to the browser window) uses the following definitions and so doesn't
clash:
{ "Help", HELP_OPEN_CONTENTS, 0 },
{ "Help.HelpContent", HELP_OPEN_CONTENTS, 0 },
{ "Help.HelpGuide", HELP_OPEN_GUIDE, 0 },
{ "_Help.HelpInfo", HELP_OPEN_INFORMATION, 0 },
{ "Help.HelpCredits", HELP_OPEN_CREDITS, 0 },
{ "_Help.HelpLicence", HELP_OPEN_LICENCE, 0 },
{ "Help.HelpInter", HELP_LAUNCH_INTERACTIVE, 0 },
The problem are the other two entries: Global History and Hotlist, as the
same tokens are used in both iconbar and main menus. The following are the
corresponding entries from the main menu definition:
{ "Utilities.Hotlist.HotlistShow", HOTLIST_SHOW, 0 },
{ "Utilities.History.HistGlobal", HISTORY_SHOW_GLOBAL, 0 },
For consistency, I /think/ my preference would be to create two new tokens
xx.ro.AppHotlistShow:Show hotlist...
xx.ro.AppHistGlobal:Show global history...
and update the iconbar menu to use these. That's consistent with AppHelp,
and indicates the Application (ie. iconbar) Menu.
--
Steve Fryatt - Leeds, England
http://www.stevefryatt.org.uk/
Re: Re: Bug 2171
On 5 Mar, "Glen Walker" wrote in message : > The accelerator may be any alphanumeric character, any function key (from > F1 to F12) or one of the special characters listed in the table below > (again, case doesn't matter): The handling of keyboard shortcuts in RISC OS menus is one of the massive bodges that the OS is famed for. Back in the day when the OS used a fixed-width font or nothing, you just included the shortcuts in the menu text and right-aligned them by hand with spaces. Help... F1 Show Global History ^F6 or whatever. When we got proportional fonts on the desktop, that clearly stopped working. But to avoid breaking existing apps, Acorn defined a set of shortcut codes and when building a menu from Wimp_CreateMenu the Wimp searches menu entries looking for them at the end of line. If it finds one, it removes all of the preceeding spaces and then right-aligns them automatically. Unfortunately, as "Red", "Blue" and "Green" are names of keys in RISC OS 5, this can have some slightly unexpected consequences for a menu item like "See Red"... -- Steve Fryatt - Leeds, England http://www.stevefryatt.org.uk/
Re: Re: Bug 2171
On 5 Mar, "Glen Walker" wrote in message
:
> Steve suggested I take a look at Bug 2171
> (http://bugs.netsurf-browser.org/mantis/view.php?id=2171) as a way of
> getting to grips with the source code.
>
> I've been looking and scratching my head for a day or so.
When I suggested looking at ticket #2171, I also said that I'd put together
my thoughts on how to go about fixing it -- but that it would have to wait
for a day or two due to lack of time. This thread seems to have pre-empted
that (and I've already explained those thoughts in another post), so I won't
bother now. :-)
However, I also thought that iconbar.c/h was a useful bit of code to look at
in terms of the more general structure of the RISC OS front-end. Since I'd
already written the best part of some notes on that, I've completed them and
here they are for what use they may be.
If people think it's a good idea, I'll try and transfer these to the Wiki
along with the file list that I posted the other day.
As ever, please ask questions... :-)
The Iconbar
---
NetSurf's iconbar icon and menu are quite good as a code example, as they're
relatively recent in their current implementation and so show off the
event-driven structure that's the preferred way to add new features (or
upgrade old ones). All of the code lives in the iconbar.c/h files, inside
workspace/netsurf/riscos.
A quick check of iconbar.h reveals that it only presents two functions to
the outside world: ro_gui_iconbar_initialise() and
ro_gui_iconbar_check_menu(). It also shows that someone failed to comment
the file properly (although the two functions are documented in iconbar.c).
Most code modules in NetSurf have an initialise function, which in the
front-end will be called by gui_init() to give them chance to set themselves
up; iconbar.c is no exception.
If we have a look at the definiton of ro_gui_iconbar_initialise() in
iconbar.c, we'll find something that's very similar to what's in many of the
RISC OS front-end's newer modules. The iconbar icon's a simple example, but
the steps are generally the same.
The first thing the code does is build the iconbar menu; this is the bit
we're interested in for the fix:
/* Build the iconbar menu */
static const struct ns_menu iconbar_definition = {
"NetSurf", {
{ "Info", NO_ACTION, &dialog_info },
{ "AppHelp", HELP_OPEN_CONTENTS, 0 },
{ "Open", BROWSER_NAVIGATE_URL, 0 },
{ "Open.OpenURL", BROWSER_NAVIGATE_URL, &dialog_openurl },
{ "Open.HotlistShow", HOTLIST_SHOW, 0 },
{ "Open.HistGlobal", HISTORY_SHOW_GLOBAL, 0 },
{ "Open.ShowCookies", COOKIES_SHOW, 0 },
{ "Choices", CHOICES_SHOW, 0 },
{ "Quit", APPLICATION_QUIT, 0 },
{NULL, 0, 0}
}
};
ro_gui_iconbar_menu = ro_gui_menu_define_menu(&iconbar_definition);
The definition is a two-stage process. The first is to define a struct
ns_menu which contains the NetSurf definition of the menu. The second is to
pass this definition to ro_gui_menu_define_menu(), which turns it into an
OSLIb wimp_menu; in other words, a RISC OS menu definition that can be
passed to the Wimp_CreateMenu SWI to display the menu. The RISC OS front-end
uses OSLib for all its interaction with the OS. ro_gui_menu_define_menu()
also adds the menu to menus.c's own list of menus, to enable selection
decoding.
The top-level structure of NetSurf's menu definition contains a token for
the menu title and a list of entries. The entries each consist of three
parts:
- A .-separated list of tokens for the entries. Each . takes us down a
submenu level, so the example above would create a top-level menu containing
tokens "Info", "AppHelp", "Open", "Choices" and "Quit". "Open" would lead to
a submenu with the title from token "Open", and entries "OpenURL",
"HotlistShow", "HistGlobal" and "ShowCookies". A menu separator can be
inserted by prefixing the string with _ (eg. "_Open.HistGlobal" to put a
separator in the sumenu.
- An action token. This is from the typedef enum menu_action defined in
menus.h, and is used to refer to that menu entry throughout the front-end.
Menu_Select events (see later) will return this code to report user choices.
- A pointer to a dialogue box, or 0 for none. "Info" leads to dialog_info,
which is NetSurf's standard RISC OS Program Information window.
If a menu entry leads to a submenu or dialogue box, it will automatically
have submenu warnings turned on (so it will generate Menu_warning events:
see later).
Next the code creates an iconbar icon:
/* Create an iconbar icon. */
wimp_icon_create icon = {
wimp_ICON_BAR_RIGHT,
{ { 0, 0, 68, 68 },
wimp_ICON_SPRITE | wimp_ICON_HCENTRED | wimp_ICON_VCENTRED |
(wimp_BUTTON_CLICK << wimp_ICON_BUTTON_TYPE_SHIFT),
{ "!netsurf" } } };
error = xwimp_create_icon(&icon, 0);
if (error) {
LOG(("xwimp_create_icon: 0x%x: %s",
error->errnum, error->errmess));
die(error->errmess);
}
Here we create an OSLib wimp_icon_create structure,
Re: Bug 2171
On Thu, 5 Mar 2015 22:55:59 +, Steve Fryatt wrote:
> The problem are the other two entries: Global History and Hotlist, as the
> same tokens are used in both iconbar and main menus. The following are the
> corresponding entries from the main menu definition:
>
> { "Utilities.Hotlist.HotlistShow", HOTLIST_SHOW, 0 },
> { "Utilities.History.HistGlobal", HISTORY_SHOW_GLOBAL, 0 },
>
> For consistency, I /think/ my preference would be to create two new tokens
>
> xx.ro.AppHotlistShow:Show hotlist...
> xx.ro.AppHistGlobal:Show global history...
>
> and update the iconbar menu to use these. That's consistent with AppHelp,
> and indicates the Application (ie. iconbar) Menu.
When I originally wrote the Amiga frontend, I re-used as many of the
existing strings as possible. As such, there are already copies of
some of the RISC OS menu strings with the shortcuts stripped out.
They are all suffixed NS (for No Shortcut), and "Show hotlist..." and
"Show global history..." are definitely present.
They might be set to ami only, but most of the old strings are set for
all platforms, so the chances are they're already there.
Chris
Re: Re: Bug 2171
Thanks Steve that's brilliant work! :--) I'm away this weekend but hopefully next week I'll get some time to digest what you have written and maybe even squash 2171 - if I do any bug fixing, how would I submit the revisions? > However, I also thought that iconbar.c/h was a useful bit of code to look at > in terms of the more general structure of the RISC OS front-end. Since I'd > already written the best part of some notes on that, I've completed them and > here they are for what use they may be. > > If people think it's a good idea, I'll try and transfer these to the Wiki > along with the file list that I posted the other day. > > As ever, please ask questions... :-) >
