Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unified support for add-on documentation #2694

Closed
nvaccessAuto opened this issue Oct 1, 2012 · 32 comments
Closed

Unified support for add-on documentation #2694

nvaccessAuto opened this issue Oct 1, 2012 · 32 comments
Assignees
Labels
Addon/management In NVDA management of addons by users. enhancement
Milestone

Comments

@nvaccessAuto
Copy link

Reported by jteh on 2012-10-01 02:41
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.
Blocking #3917

@nvaccessAuto
Copy link
Author

Comment 1 by norrumar on 2012-10-03 19:45
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.

@nvaccessAuto
Copy link
Author

Comment 2 by norrumar on 2012-10-04 21:44
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.

@nvaccessAuto
Copy link
Author

Comment 3 by jteh on 2012-10-09 06:40
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.

@nvaccessAuto
Copy link
Author

Comment 4 by norrumar on 2012-10-09 09:32
@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.

@nvaccessAuto
Copy link
Author

Comment 5 by norrumar on 2012-10-09 19:43
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.

@nvaccessAuto
Copy link
Author

Comment 6 by jteh (in reply to comment 5) on 2012-10-10 06:51
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.

@nvaccessAuto
Copy link
Author

Comment 7 by ragb (in reply to comment 6) on 2012-10-10 14:00
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.

@nvaccessAuto
Copy link
Author

Comment 8 by norrumar on 2012-10-10 18:09
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.

@nvaccessAuto
Copy link
Author

Attachment 2694.diff added by norrumar on 2012-10-10 18:11
Description:
Bzr bundle for this ticket

@nvaccessAuto
Copy link
Author

Comment 9 by MHameed on 2013-05-20 06:02
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.

@nvaccessAuto
Copy link
Author

Comment 12 by jteh on 2013-05-29 02:05
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.

@nvaccessAuto
Copy link
Author

Comment 14 by norrumar on 2013-06-17 19:53
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.

@nvaccessAuto
Copy link
Author

Comment 15 by jteh on 2013-06-20 04:43
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.

@nvaccessAuto
Copy link
Author

Comment 16 by MHameed on 2013-06-20 06:56
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//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.

@nvaccessAuto
Copy link
Author

Comment 17 by jteh on 2014-02-21 03:09
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.

@nvaccessAuto
Copy link
Author

Comment 18 by norrumar (in reply to comment 17) on 2014-02-21 04:28
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.

@nvaccessAuto
Copy link
Author

Comment 20 by ateu on 2014-02-22 12:28
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.

@nvaccessAuto
Copy link
Author

Comment 21 by norrumar on 2014-02-22 16:17
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.

@nvaccessAuto
Copy link
Author

Comment 22 by norrumar on 2014-02-23 05:53

  • 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.

@nvaccessAuto
Copy link
Author

Comment 24 by jteh on 2014-05-26 04:15
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.
Changes:
Changed title from "Unified support for add-on documentation localisation" to "Unified support for add-on documentation"

@nvaccessAuto
Copy link
Author

Comment 25 by norrumar (in reply to comment 24) on 2014-05-26 05:14
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.

@nvaccessAuto
Copy link
Author

Comment 26 by jteh on 2014-05-27 07:58
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?

@nvaccessAuto
Copy link
Author

Comment 27 by jteh on 2014-05-27 07:58
Changes:
Milestone changed from near-term to next

@nvaccessAuto
Copy link
Author

Comment 28 by norrumar (in reply to comment 26) on 2014-05-30 15:10
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.

@nvaccessAuto
Copy link
Author

Comment 29 by jteh (in reply to comment 28) on 2014-06-02 03:39
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.

@nvaccessAuto
Copy link
Author

Comment 30 by norrumar (in reply to comment 29) on 2014-06-02 05:47
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.

@nvaccessAuto
Copy link
Author

Comment 32 by James Teh <jamie@... on 2014-06-03 04:31
In [30dc2eb]:

Merge branch 't2694' into next

Incubates #2694.

Changes:
Added labels: incubating

@nvaccessAuto
Copy link
Author

Comment 33 by James Teh <jamie@... on 2014-06-24 08:56
In [2347833]:

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

Changes:
Removed labels: incubating
State: closed

@nvaccessAuto
Copy link
Author

Comment 34 by jteh on 2014-06-24 08:58
Changes:
Milestone changed from next to 2014.3

@nvaccessAuto
Copy link
Author

Comment 35 by driemer.riemer@... on 2014-08-20 02:57
Hey, is this in the dev guide yet? it isn't on the website.

@nvaccessAuto
Copy link
Author

Comment 36 by jteh on 2014-08-20 06:00
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.

@nvaccessAuto
Copy link
Author

Comment 37 by driemer.riemer@... on 2014-08-20 06:32
Oh. Okay just checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Addon/management In NVDA management of addons by users. enhancement
Projects
None yet
Development

No branches or pull requests

2 participants