Opened 2 years ago

Closed 4 months ago

Last modified 2 months ago

#2694 closed enhancement (fixed)

Unified support for add-on documentation

Reported by: jteh Owned by: jteh
Priority: minor Milestone: 2014.3
Component: Add-on management Version:
Keywords: Cc: MHameed, nrm1977@gmail.com, nvdakor
Operating system: Blocked by:
Blocking: #3917
Changes document entry (for developers): New Features: - Help for add-ons can be accessed from the Add-ons Manager for add-ons which support this. (#2694) Changes for Developers: - NVDA now has unified support for add-on documentation. See the Add-on Documentation section of the Developer Guide for details. (#2694)

Description

It's already possible for add-ons to provide their own documentation. However, there's no unified support or documentation for this, which makes it inconsistent and difficult and means constant reinvention of the wheel.

I think plugins should still add their documentation to the Help menu themselves. However, it might be good to have functions in addonHandler to make retrieving the path to localised documentation files easier and to document (probably in the Developer Guide) where add-on documentation should be placed.

Attachments (1)

2694.diff (6.8 KB) - added by norrumar 2 years ago.
Bzr bundle for this ticket

Download all attachments as: .zip

Change History (38)

comment:1 Changed 2 years ago by norrumar

I think we sould open files instead documentation folders, but I send this bundle because I have used this code into some add-ons (plugins or app modules). Function isDocFolder can be used in menus, so we can set menu available if documentation folder exists or not available otherwise.
Regards.

comment:2 Changed 2 years ago by norrumar

There are three functions added into addonHandler:

  • getDocFolder(), in the class that represents an addon.
  • docPath: It retrieves the path for readme.html, readme.txt or, if this files can not be found, the path for documentation folder corresponding to the current language. This function can be called from add-ons using addonHandler.docPath().
  • isDocFolder(): This functions returns true if documentation folder is available for the currrent language.

I think that I have finished my changes to this bundle now. Regards.

comment:3 Changed 2 years ago by jteh

Thanks for your work.

getDocFolder() should probably resort to English if there are no docs for the current locale. I also wonder whether it should raise an exception or return None if the folder doesn't exist.

Why is isDocFolder() necessary? Imo, the add-on shouldn't need to ask the question. The author knows whether it has documentation, so the code doesn't need to ask this.

docPath() should probably be renamed to getDocPath(). Also, I think it should take a file name, as the author may wish to have multiple documents. I guess this can default to readme.html, though.

Minor point: getDocFolder() shouldn't calculate tryLang unless it is necessary. Right now, it is calculated even if it isn't used.

comment:4 Changed 2 years ago by norrumar

@JT said:
getDocFolder() should probably resort to English if there are no docs for the current locale.

I'm not sure about this point. For instance most of my add-ons have documentation only in Spanish or other languages. I don't know if English sould be set as default language.

"I also wonder whether it should raise an exception or return None if the folder doesn't exist.
"

In my add-on for eMule, I use insert+h for opening a file containing help. In this module I included an exception, but getDocFolder perhaps should return None, because then you can raise different exceptions. It's just an idea.
"Minor point: getDocFolder() shouldn't calculate tryLang unless it is necessary. Right now, it is calculated even if it isn't used.".
Good thought.
"docPath() should probably be renamed to getDocPath(). Also, I think it should take a file name, as the author may wish to have multiple documents. I guess this can default to readme.html, though."
OK. I have writen this code in different ways. OK.
"Why is isDocFolder() necessary? Imo, the add-on shouldn't need to ask the question. The author knows whether it has documentation, so the code doesn't need to ask this.".
It is not necessary, but for me is easier to work in this way. Author knows if her or his add-on has documentation, but other people can add translation an author perhaps does not know this fact.
I use this code in an add-on named selectedElements:

self.aboutItem = self.selectedElementsMenu.Append(wx.ID_ANY, _("Open &information folder"), _("Open documentation for current language"))
self.aboutItem.Enable(self.isDocFolder())

So Help for this add-on is allways added in the menu, but it is not enabled if language documentation does not exist for the current language.
Thanks.

comment:5 follow-up: Changed 2 years ago by norrumar

I have done changes in addonHandler:

  • The function which retrieves the path containing the first file that should be opened for getting help is named getDocPath. We dont need to pass values as parameters, though it accept a file name and a list of extensions. By default the file name is "readme" and extensions are "html" and "txt. So we search "readme.html" and, if this file is not found, "readme.txt". If no file is found, getDocPath retrieves the corresponding doc folder, which can contain files in other formats, that should be specified in the plugin code, though it is not necessary.
  • In this version, isDocFolder also returns True or False, as function for enabling menu items.
  • getDocFolder returns None if we can not find the corresponding documentation folder, and tries to retrieve "en" if documentation folder is not found for current language.

Regards.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 2 years ago by jteh

Replying to norrumar:

getDocFolder() should probably resort to English if there are no docs for the current locale.

I'm not sure about this point. For instance most of my add-ons have documentation only in Spanish or other languages. I don't know if English sould be set as default language.

Because NVDA's primary language is English, this bias already exists in the code. For example, strings enclosed in _() are expected to be English. The user shouldn't notice this, of course, but it's consistent in the code.

Why is isDocFolder() necessary? Imo, the add-on shouldn't need to ask the question. The author knows whether it has documentation, so the code doesn't need to ask this.

Author knows if her or his add-on has documentation, but other people can add translation an author perhaps does not know this fact.

Hmm. Tricky. For NVDA's own documentation, it always resorts to English if no documentation is available in the user's language. While this might seem odd, it means that the user at least has the option of reading the documentation in English (assuming they can understand it), rather than being given no access at all because it hasn't been translated. I'd like to hear others' thoughts on this for add-ons.

In any case, isDocFolder should probably be renamed to hasDocFolder or maybe even just hasDoc.

Replying to norrumar:

  • The function which retrieves the path containing the first file that should be opened for getting help is named getDocPath. We dont need to pass values as parameters, though it accept a file name and a list of extensions. By default the file name is "readme" and extensions are "html" and "txt.

This seems unnecessarily complicated. Imo, it only needs to take a file name. The add-on author should know the name of the file and translators should be producing documentation in the same format.

comment:7 in reply to: ↑ 6 Changed 2 years ago by ragb

Replying to jteh:

Replying to norrumar:

getDocFolder() should probably resort to English if there are no docs for the current locale.

I'm not sure about this point. For instance most of my add-ons have documentation only in Spanish or other languages. I don't know if English sould be set as default language.

Because NVDA's primary language is English, this bias already exists in the code. For example, strings enclosed in _() are expected to be English. The user shouldn't notice this, of course, but it's consistent in the code.

IMO, it makes no sence to not have documentation in english, if it is to be translated somehow. And english seems to me the only reasonable defaut to revert to, if the user's language is not available.

What I'm trying to say is that I strongly recomend if the add-on has documentation it has to be writen at least in english. If the author is not a native speaker, I supose there are people available to take a look at gramar and such, if needed.

Hmm. Tricky. For NVDA's own documentation, it always resorts to English if no documentation is available in the user's language. While this might seem odd, it means that the user at least has the option of reading the documentation in English (assuming they can understand it), rather than being given no access at all because it hasn't been translated. I'd like to hear others' thoughts on this for add-ons.

I fully agree with this. It's better english then nothing at all, by all means.

In any case, isDocFolder should probably be renamed to hasDocFolder or maybe even just hasDoc.

Agree. isDocFolder seems like a boolean property of some object.

Replying to norrumar:

  • The function which retrieves the path containing the first file that should be opened for getting help is named getDocPath. We dont need to pass values as parameters, though it accept a file name and a list of extensions. By default the file name is "readme" and extensions are "html" and "txt.

This seems unnecessarily complicated. Imo, it only needs to take a file name. The add-on author should know the name of the file and translators should be producing documentation in the same format.

I agree. The only suitable default I can think of is index.html if the docs are in html at all. And this is very personal.

My only concern with this aproach of beeing the add-on author responsible of adding items to the help menu, is that the order of items in this menu will not be previsible. The problem is that there may not be a suitable order actualy. I'd say alfabetical order of add-on name, but that is just random too.

comment:8 Changed 2 years ago by norrumar

I agree with you: it's better to have at least a language into which documentation must be translated, in this case English.
Then I think we don't hasDoc or isDocFolder, because, if an add-on has documentation, this help will be available in English, which is set as default language.
I thought that we could include a list of posible names as the first parameter of getDocPath too for flexibility. But if in your opinion a list of extensions is tricky, it's OK for me. I don't know what people prefer, so I have deleted the second parameter for getDocPath. Now it takes only file name, by default "readme.txt".
Thanks.

Changed 2 years ago by norrumar

Bzr bundle for this ticket

comment:9 Changed 18 months ago by MHameed

sorry what is currently outstanding for this?
Re English or not, I would say yes, if it is going to be a community supported addon it will always need English documentation.
Saying that, I do understand where Nolia is coming from.

Last edited 18 months ago by MHameed (previous) (diff)

comment:10 Changed 18 months ago by MHameed

  • Cc MHameed added

comment:11 Changed 18 months ago by jteh

  • Keywords needsCodeReview added

comment:12 Changed 17 months ago by jteh

I've made a few changes to this code and added documentation.

@norrumar, can you please provide your full name so we can acknowledge you as a contributor and copyright holder for this code? Thanks.

comment:13 Changed 17 months ago by jteh

  • Keywords needsCodeReview removed

comment:14 Changed 17 months ago by norrumar

Replying to @jt
"
@norrumar, can you please provide your full name so we can acknowledge you as a contributor and copyright holder for this code? Thanks.
"
My full name is Noelia Ruiz Martínez. You can just use my first name if you want.
Excuse me, I have seen your question now thanks to Mesar.
Regards.

comment:15 Changed 17 months ago by jteh

Mesar suggests there is still demand to make adding of add-on documentation to the Help menu more automatic. The add-on template now implements its own hack to do this, but if everyone is going to use this, it'd make sense to do it elegantly in the core. This suggests we should have some sort of mix-in utility class to handle this. This could only handle the most common case; i.e. a simple readme.html document.

comment:16 Changed 17 months ago by MHameed

adding the functionality to core along the lines of our discussion would be great and cleaner than in addonTemplate.
As long as the default case can open up doc/<currentNVDALanguage>/readme.html this will be enough for the majority of addons.

Nolia and I have sorted out the automatic generation of html documentation with no additional work to translators, so the task is almost complete from this side.

comment:17 follow-up: Changed 8 months ago by jteh

Actually, I still don't see why this needs to be totally automatic. If we implement a helper function, it's really not that hard to call a couple of functions in the right places. It saves wasted checks when they aren't necessary; e.g. if someone does documentation differently to the way prescribed by the template.

The global plugin in the add-on template really needs to be removed, as it affects performance, both at startup and every time NVDA processes a key press or event. There's no need for the documentation for an app module to be available even when the application isn't running. It could be made into a utility module which gets used by add-on code.

comment:18 in reply to: ↑ 17 Changed 8 months ago by norrumar

Replying to jteh:
OK, I think that adding a menu item automatically could be more consistent, but not to do it provides more flexibility.
I agree with you: docHandler must be removed from add-on template. I hope you can provide functions to manage documentation in addonHandler.
We can work on a branch for this if necessary.
As aside, there are add-on, for instance dropbox, that can use globalPlugin an app modules. If it's not recommended, hope it can be documented in the developer guide.
Another think: as you say in the summary and description of this ticket, documentation can be related to add-ons, not just to each component (app module, global plugin or driver). Furthermore, NVDA user guide contain information about functions provided in app modules, and it's useful though they are not used: knowing these features before opening an application can help to decide if this will be used.
Perhaps documentation for add-ons could be included in the add-ons manager, as the about button or something similar. So if an add-on has no documentation, this button could be disabled, for instance.
Thanks.

Actually, I still don't see why this needs to be totally automatic. If we implement a helper function, it's really not that hard to call a couple of functions in the right places. It saves wasted checks when they aren't necessary; e.g. if someone does documentation differently to the way prescribed by the template.

The global plugin in the add-on template really needs to be removed, as it affects performance, both at startup and every time NVDA processes a key press or event. There's no need for the documentation for an app module to be available even when the application isn't running. It could be made into a utility module which gets used by add-on code.

Last edited 8 months ago by norrumar (previous) (diff)

comment:19 Changed 8 months ago by norrumar

  • Cc nrm1977@gmail.com added

comment:20 Changed 8 months ago by ateu

As I said in #3917, there is a potential problem on adding the menu items directly in Help Menu. If an user has installed several add-ons, the Help Menu will become so larger, since will be have the original help menu items and the menu items from the add-ons.
For this reason,I suggest a sub-menu in the Help Menu, which can be called add-ons documentation, where the user will find the documentation for all add-ons.

comment:21 Changed 8 months ago by norrumar

A t2694 branch has been created for this ticket at
https://bitbucket.org/nvdaaddonteam/nvda/

  • It has the getDocFolder and getDocPath functions into the addon class.
  • An openDocPath() method has been added to this class too.
  • In an optional docname field, contained in the manifest add-on, can be specified the default name for the addon documentation file.

Thanks.

comment:22 Changed 8 months ago by norrumar

  • A hasDoc() method has been added to check if the add-on has or not the documentation specified in addon.getDocPath().
  • getDocPath() retrieves the documentation file, and not the folder if a file (not an empty string, "") is specified in the add-on manifest.

I think my work is finished for this branch if nothing is required.
Thanks.

comment:23 Changed 5 months ago by jteh

  • Blocking 3917 added

comment:24 follow-up: Changed 5 months ago by jteh

  • Cc nvdakor added
  • Summary changed from Unified support for add-on documentation localisation to Unified support for add-on documentation

There is clear demand for unified access to add-on documentation, so I'm broadening the scope of this ticket accordingly.

First, thanks Noelia for your work on this. Sorry it's taken us so long to actually do anything useful with it. I'm going to try to devote some time to code review each week from now on, so hopefully, we can get this moving a bit.

Regarding the current code:

  • I'm not sure docname should have a default. I know the call is really inexpensive, but it seems inefficient to have to check whether documentation exists when the add-on author can just specify whether it exists or not, especially since we're introducing a manifest entry anyway. I suggest it defaults to None, where None means there is no documentation.
  • Cosmetically, I'd prefer docName (capital n) instead of docname. Actually, docFileName might be more descriptive.

Are there any major objections to accessing the documentation from the Add-ons Manager as outlined in #3917? I suggest, though, that we have a separate "Help" or "Documentation" button in the Add-ons Manager to avoid having to press "About" first, thus eliminating one step.

comment:25 in reply to: ↑ 24 Changed 5 months ago by norrumar

Replying to jteh:

There is clear demand for unified access to add-on documentation, so I'm broadening the scope of this ticket accordingly.

First, thanks Noelia for your work on this. Sorry it's taken us so long to actually do anything useful with it. I'm going to try to devote some time to code review each week from now on, so hopefully, we can get this moving a bit.

Regarding the current code:

  • I'm not sure docname should have a default. I know the call is really inexpensive, but it seems inefficient to have to check whether documentation exists when the add-on author can just specify whether it exists or not, especially since we're introducing a manifest entry anyway. I suggest it defaults to None, where None means there is no documentation.

OK, it seams reasonable.

  • Cosmetically, I'd prefer docName (capital n) instead of docname. Actually, docFileName might be more descriptive.

OK, I agree. docFileName is better.

Are there any major objections to accessing the documentation from the Add-ons Manager as outlined in #3917? I suggest, though, that we have a separate "Help" or "Documentation" button in the Add-ons Manager to avoid having to press "About" first, thus eliminating one step.

I strongly agree with you about accessing documentation from add-ons manager with a separated button, without pressing About.
Thanks.

comment:26 follow-up: Changed 5 months ago by jteh

I've refactored this implementation a bit and exposed it in the GUI. See the t2694 branch in the main NVDA repo.

There is now just one method on addonHandler.Addon called getDocFilePath. It takes an optional file name, defaulting to the docFileName parameter in the manifest. docFileName defaults to None. I'm not sure if anyone will ever want to open a file other than the default. If they do, it's possible that only some of the documentation is localised for a given language, so it needs to fall back to other languages on a per-file basis, not a per-directory basis. I also figured this is more useful than getting the documentation folder alone.

I removed the method to open the documentation because the caller might have already retrieved the file path to check whether it exists, so getting it twice is wasteful. Also, they might want to handle errors in a specific way. If you think this is still necessary, let me know.

The Add-on help button in the Add-ons Manager will open the default documentation file if there is one. If there isn't, the button will be disabled.

Does this work for everyone?

comment:27 Changed 5 months ago by jteh

  • Milestone changed from near-term to next
  • Owner changed from mdcurran to jteh
  • Status changed from new to accepted

comment:28 in reply to: ↑ 26 ; follow-up: Changed 5 months ago by norrumar

Replying to jteh:

I've refactored this implementation a bit and exposed it in the GUI. See the t2694 branch in the main NVDA repo.

There is now just one method on addonHandler.Addon called getDocFilePath. It takes an optional file name, defaulting to the docFileName parameter in the manifest. docFileName defaults to None. I'm not sure if anyone will ever want to open a file other than the default. If they do, it's possible that only some of the documentation is localised for a given language, so it needs to fall back to other languages on a per-file basis, not a per-directory basis. I also figured this is more useful than getting the documentation folder alone.

I removed the method to open the documentation because the caller might have already retrieved the file path to check whether it exists, so getting it twice is wasteful. Also, they might want to handle errors in a specific way. If you think this is still necessary, let me know.

The Add-on help button in the Add-ons Manager will open the default documentation file if there is one. If there isn't, the button will be disabled.

Does this work for everyone?

For me it's right now.
Just a minor point: as you tell me in #Comment3, tryLang shouldn't be calculated if not necessary, In your branch now appears:

if "_" in lang:

lang = lang.split("_", 1)[0]

The result is appended to the langs list, but perhaps it's not necessary if the first calculated lang exist. Or am I wrong?
The second point is that I thing this code works with non latin characters, but our docHandler doesn't work without using
_addonDir = os.path.join(os.path.dirname(file), "..").decode("mbcs") # The root of an addon folder
Thanks.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 5 months ago by jteh

Replying to norrumar:

Just a minor point: as you tell me in #Comment3, tryLang shouldn't be calculated if not necessary

I don't have the original version of your patch, so I can't quite remember why I said this. I wonder whether your original code was testing for the existence of the directory even though an earlier directory existed. Adding a string to a list isn't an issue, but unnecessary file system checks are a bit more expensive. Perhaps you didn't do this and I was just being overly paranoid. Either way, the code you needed in the end to work around it (multiple checks and break statements) was more confusing than just adding the string to the list, so I think adding a string to a list is better. I apologise if I indeed gave you the wrong advice earlier. :)

The second point is that I thing this code works with non latin characters, but our docHandler doesn't work without using
_addonDir = os.path.join(os.path.dirname(file), "..").decode("mbcs") # The root of an addon folder

What you're doing there is converting an ANSI path to a Unicode path, since Python modules use ANSI paths. However, the paths returned by Addon.path are already Unicode. Out of interest, how does docHandler fail without the decode? Most functions should be able to accept both ANSI and Unicode paths.

comment:30 in reply to: ↑ 29 Changed 5 months ago by norrumar

Replying to jteh:

Replying to norrumar:

Just a minor point: as you tell me in #Comment3, tryLang shouldn't be calculated if not necessary

I don't have the original version of your patch, so I can't quite remember why I said this. I wonder whether your original code was testing for the existence of the directory even though an earlier directory existed. Adding a string to a list isn't an issue, but unnecessary file system checks are a bit more expensive. Perhaps you didn't do this and I was just being overly paranoid. Either way, the code you needed in the end to work around it (multiple checks and break statements) was more confusing than just adding the string to the list, so I think adding a string to a list is better. I apologise if I indeed gave you the wrong advice earlier. :)

OK, I don't have my original code, but I agree with you. In this case, your code is right for me.

The second point is that I thing this code works with non latin characters, but our docHandler doesn't work without using
_addonDir = os.path.join(os.path.dirname(file), "..").decode("mbcs") # The root of an addon folder

What you're doing there is converting an ANSI path to a Unicode path, since Python modules use ANSI paths. However, the paths returned by Addon.path are already Unicode. Out of interest, how does docHandler fail without the decode? Most functions should be able to accept both ANSI and Unicode paths.

I can't test why docHandler doesn't work withouu ansi to unicode conversion, since my name has just latin characters. However, I test your t2694 branch using non latin characters in docFileName and it works perfectly.
We have an add-on named placeMarkers and it needs ansi to unicode conversion too. Otherwise for instance cyrillic characters are not supported to save bookmarks, since the file name for them is based on the title of the web page. I use mbcs codding, but I think we should try with the add-on path.
When I have time to review add-ons again, I will look all this and be pleased to remove doc handlers if you merge your branch.
I think we need just to add the documentation in developer guide and entry for developers, etc.
Thanks.

comment:31 Changed 5 months ago by jteh

  • Changes document entry (for developers) modified (diff)

comment:32 Changed 5 months ago by James Teh <jamie@nvaccess.org>

  • Status changed from accepted to incubating

In 30dc2ebfd30e7afbfbcd97b23e39e22a67bf3ffa:

Merge branch 't2694' into next

Incubates #2694.

comment:33 Changed 4 months ago by James Teh <jamie@nvaccess.org>

  • Resolution set to fixed
  • Status changed from incubating to closed

In 2347833d880ed22312ffd1edb080126cd160ad15:

Unified support for add-on documentation.

Add-ons can specify a documentation file name using the docFileName parameter in their manifests.
If an add-on has documentation, the Add-on help button in the Add-ons Manager will open this file.
Fixes #2694.
Authors: Noelia Ruiz Martínez <nrm1977@gmail.com>, James Teh <jamie@nvaccess.org>

comment:34 Changed 4 months ago by jteh

  • Milestone changed from next to 2014.3

comment:35 Changed 2 months ago by driemer.riemer@gmail.com

Hey, is this in the dev guide yet? it isn't on the website.

comment:36 Changed 2 months ago by jteh

The Developer Guide for a release isn't made available on the site until the final release. So, this won't show up on the site until 2014.3 is released.

comment:37 Changed 2 months ago by driemer.riemer@gmail.com

Oh. Okay just checking.

Note: See TracTickets for help on using tickets.