First installment of patch. Includes new help module and some minor changes in system.module to reflect new api/theme functions.

Other features will be introduced with next patches as it would be easier to maintain patches this way.

Files: 
CommentFileSizeAuthor
#306 drupal7-help-system_299050-306.patch112.87 KBGurpartap Singh
Passed: 10421 passes, 0 fails, 0 exceptions View
#305 drupal7-help-system_299050-305.patch112.87 KBGurpartap Singh
Passed: 10421 passes, 0 fails, 0 exceptions View
#304 drupal7-help-system_299050-304.patch113.23 KBGurpartap Singh
Passed: 10421 passes, 0 fails, 0 exceptions View
#295 drupal7-help-system_299050-295.patch115.24 KBGurpartap Singh
Passed: 10421 passes, 0 fails, 0 exceptions View
#293 help_code.patch40.91 KBFrando
Failed: Failed to apply patch. View
#293 help_helpfiles.patch74.9 KBFrando
Passed: 10551 passes, 0 fails, 0 exceptions View
#291 drupal7-help-system_299050-291.patch114.95 KBGurpartap Singh
Unable to apply patch drupal7-help-system_299050-291.patch View
#289 drupal7-help-system_299050-289.patch115.74 KBGurpartap Singh
Failed: Failed to apply patch. View
#288 help_code_2.patch41.53 KBFrando
Failed: Failed to apply patch. View
#288 help_helpfiles.patch74.76 KBFrando
Failed: 10609 passes, 1 fail, 0 exceptions View
#284 help_code.patch40.21 KBFrando
Failed: 10377 passes, 2 fails, 2 exceptions View
#284 help_helpfiles.patch74.76 KBFrando
Passed: 10551 passes, 0 fails, 0 exceptions View
#279 drupal7-help-299050-3.patch118.75 KBredndahead
Passed: 10421 passes, 0 fails, 0 exceptions View
#277 drupal7-help-299050-2.patch118.71 KBredndahead
Unable to apply patch drupal7-help-299050-2.patch View
#268 drupal7-help-299050-1.patch115.2 KBredndahead
Passed: 10421 passes, 0 fails, 0 exceptions View
#267 help-299050-1.patch115.2 KBredndahead
Passed: 10421 passes, 0 fails, 0 exceptions View
#238 drupal7-help-system_299050-238.patch123.48 KBGurpartap Singh
Failed: Failed to apply patch. View
#236 drupal7-help-system_299050-236.patch65.39 KBadd1sun
Failed: Failed to apply patch. View
#232 drupal7-help-system_299050-232.patch62.83 KBadd1sun
Failed: 9621 passes, 3 fails, 1 exception View
#232 core-help-files-232.tgz38.43 KBadd1sun
#226 drupal7-help-system_299050-226.patch48 KBadd1sun
Failed: 9581 passes, 3 fails, 2 exceptions View
#225 drupal7-help-system_299050-225.patch47.96 KBadd1sun
Failed: 9581 passes, 3 fails, 2 exceptions View
#224 drupal7-help-system_299050-224.patch47.15 KBadd1sun
Failed: 9581 passes, 3 fails, 2 exceptions View
#213 itsy_bitsy_help_0.patch121.12 KBcatch
Failed: Failed to apply patch. View
#210 itsy_bitsy_help.patch121.24 KBchx
Failed: Failed to apply patch. View
#208 itsy_bitsy_help.patch123.07 KBchx
Failed: Failed to apply patch. View
#206 drupal7-help-system_299050-206.patch41.61 KBadd1sun
Failed: 9727 passes, 78 fails, 26 exceptions View
#201 drupal7-help-system_299050-201.patch43.65 KBGurpartap Singh
Failed: 9668 passes, 70 fails, 0 exceptions View
#201 core-help-files.zip65.24 KBGurpartap Singh
#159 core-help-files.zip65.24 KBadd1sun
#159 drupal7-help-system_299050-159.patch45.02 KBadd1sun
Failed: 9366 passes, 62 fails, 0 exceptions View
#150 drupal7-help-system-299050.patch45.14 KBGurpartap Singh
Failed: 9381 passes, 63 fails, 0 exceptions View
#150 help_data.tar_.gz7.55 KBGurpartap Singh
#148 drupal7-help-system_299050-148.patch45.52 KBadd1sun
Failed: 9407 passes, 18 fails, 0 exceptions View
#137 drupal7-help-system_299050-137.patch40.74 KBadd1sun
Failed: 9273 passes, 62 fails, 22 exceptions View
#137 example-help-files.zip8.59 KBadd1sun
#137 gradient_1.png224 bytesadd1sun
#123 advanced_help.help_.txt315 bytesmerlinofchaos
#123 ini-file.html4.25 KBmerlinofchaos
#123 translation.html942 bytesmerlinofchaos
#123 translation.html942 bytesmerlinofchaos
#123 using-advanced-help.html2.32 KBmerlinofchaos
#123 why-advanced-help.html6.91 KBmerlinofchaos
#115 drupal7-help-system_299050.patch45.44 KBGurpartap Singh
Failed: 8549 passes, 18 fails, 0 exceptions View
#115 help_files.zip15.74 KBGurpartap Singh
#115 gradient.png224 bytesGurpartap Singh
#103 drupal7-help-system_299050.patch45.63 KBGurpartap Singh
Failed: 8549 passes, 18 fails, 0 exceptions View
#103 gradient.png224 bytesGurpartap Singh
#103 help_files.zip7.52 KBGurpartap Singh
#101 about-files.tar_.gz43.01 KBkeith.smith
#86 299050-86.patch47.66 KBkeith.smith
Failed: Failed to apply patch. View
#86 gradient.png224 byteskeith.smith
#86 help-about-files.tar_.gz26.37 KBkeith.smith
#85 help-about-files.tar_.gz26.37 KBkeith.smith
#84 mkdir-help-directories.sh_.txt545 byteskeith.smith
#83 299050-82.patch45.34 KBkeith.smith
Failed: Failed to apply patch. View
#75 blocks.tar_.gz50.33 KBredndahead
#75 blogs.tar_.gz77.05 KBredndahead
#75 book.tar_.gz189.21 KBredndahead
#75 contact.tar_.gz207.81 KBredndahead
#75 forum.tar_.gz265.33 KBredndahead
#75 menu.tar_.gz226.08 KBredndahead
#75 modules.tar_.gz66.25 KBredndahead
#75 pages.tar_.gz602.17 KBredndahead
#75 polls.tar_.gz212.69 KBredndahead
#75 user.tar_.gz142.84 KBredndahead
#70 about.html2.74 KBkeith.smith
#63 drupal7-feature_help-system_299050.patch6.63 KBGurpartap Singh
Failed: 9665 passes, 11 fails, 0 exceptions View
#61 drupal7-feature_help-system_299050.patch6.64 KBdmitrig01
Passed: 10551 passes, 0 fails, 0 exceptions View
#59 drupal7-feature_help-system_299050.patch6.58 KBGurpartap Singh
Failed: Failed to apply patch. View
#59 help.zip15.58 KBGurpartap Singh
#52 help.zip15.46 KBGurpartap Singh
#52 drupal7-feature_help-system.patch6.64 KBGurpartap Singh
Failed: Failed to apply patch. View
#45 299050.patch39.01 KBRobLoach
Failed: Failed to apply patch. View
#40 help.tar_.gz9.75 KBGurpartap Singh
#40 drupal7-feature_help-system.patch6.8 KBGurpartap Singh
Failed: Failed to apply patch. View
help.zip2.94 KBGurpartap Singh
drupal7-feature_help-system.patch36.62 KBGurpartap Singh
Failed: Failed to apply patch. View

Comments

keith.smith’s picture

Yay!

I've only read through the patch at this point (and have not applied it and tried it out in situ), but on a read-through this is very exciting. There's a very few code style violations (comments without periods, and the like), but those are absolutely no big deal at this point.

I'll try this out more completely at my next opportunity, but I'm very excited about this. Good job, Gurpartap!

redndahead’s picture

Status: Needs review » Needs work

I don't like using path: etc. for the token. I would prefer it to be &path&. This would allow the token to be entered anywhere not just within a src or href value. I like & because if you need to write &path&, for example in the help file for this help module, then you can do &path&

Gurpartap Singh’s picture

Status: Needs work » Needs review

I would like to collect more opinions before preparing a new patch.

keith.smith’s picture

As a general question, how do you see what you've done here interfacing with what Earl has done with his advanced_help module, or do you see a place for that? There are parts about both systems that I really like....

Gurpartap Singh’s picture

I feel earl actually owns this patch(advhelp based), I'm masquerading him. ;)

keith.smith’s picture

I've often wished I could channel Earl, so I envy you there. Thanks for the note.

merlinofchaos’s picture

An initial quick review:

  1. This patch probably needs to come with a longer explanation of the features provided by this change. It's not completely clear from the patch (and I know what's in there). Bullet points of what should be expected should be provided.
  2. I somehow expected this to have more of the welcome page structure from http://groups.drupal.org/node/13270 rather than just using a help file. I think that is one of the key innovations you've really added to my base code that really helps hit this out of the park.
  3. I expected hook_help() to be completely gone.
  4. I expected all the existing module help to have been moved to help files.
  5. I'm pretty much okay with redndahead's suggestion of using &help& for the tokens.
  6. Should theme('more_help_link') just be theme('help')?

    That's what I can think of for now.

JohnAlbin’s picture

Adding this to my short list of things I want to see in D7. :-)

hook_help is a 2nd class citizen in D6. Even the menu system refuses to add help pages to admin/by-module if your module owns no other pages. And the location of help on a page is awful. :-p

webchick’s picture

Subscribing. This is also on my (long) list of things I want to see in D7. :) No time for a thorough review now, though. I will try and make time on the weekend.

moshe weitzman’s picture

subscribe ... A few screenshots would be lovely.

Xano’s picture

  1. I'd like to see multipage help in D7. This way (compicated) modules (like Views) can offer some basic 'tutorials' through the Drupal help pages.
  2. To prevent hook_help() from becoming too cluttered perhaps we could move all help to a special help include file, like module_name.help.
  3. Separate help for users from help for administrators. Most help will be intended for administrators, but Filter tips are an example of help for normal users.

Haven't tried the patch yet, will try to do so this weekend.

Gurpartap Singh’s picture

Status: Needs review » Needs work
redndahead’s picture

xano

current patch should take care of at least concerns 1 and 2.

Xano’s picture

Still haven't tried your patch, but I did take a closer look at it:

  1. Why using pop-ups? I think pop-ups should be avoided at all times. Most people find them annoying and they don't always work.
  2. Why the HTML file for module help? Like I said in #244090: Tie help into menu router, this requires parsing the HTML file rather than just getting the strings from a PHP function.
  3. HTML is being used without putting it in a theme() function. Bad idea.

I'm still curious as to how modules should define help pages. In the other issue robertDouglass proposed using the menu system. I started rewriting help.module myself last night and I must say his idea definitely works good and it has a lot of potential. I'll post the preliminary version of my code there.

catch’s picture

#2. it should allow people without any php knowledge to contribute help files. Or at least that's what I'd hope.

Xano’s picture

But it doesn't allow for dynamic help files and on top of that they would have to be translated in a completely different way, inconsistent with normal strings.

redndahead’s picture

#1 the main complaint people have of help is that it takes you to another page. The pop ups eliminate that. Pop ups are not the devil. They have their purpose and this is a prime example of when popups should be used.

#2 The patch currently doesn't do it, but I don't think there is a reason why you can't pass the files contents, after the tokens are replaced, into a t() function. I'm pretty sure this would be the same as help is doing now.

Xano’s picture

#2 HTML files are static and cannot contain PHP, unless the contents are extracted and evaluated through eval(), which is a no go the way I see it.

merlinofchaos’s picture

Unfortunately, Gurpartap didn't post an explanation of this module, which answers all of these questions. I asked him to but he did not. Gurpartap: You *must* post a full explanation here of what this module does and why these decisions were made, or basically this is going to stall because explaining this stuff again is going to demoralize folks.

1. Why using pop-ups? I think pop-ups should be avoided at all times. Most people find them annoying and they don't always work.

If you are on a form, and you have entered data, and you click a 'help' link, and it takes you to another page, your user loses data. That is more annoying than a popup. Honestly, the attitude that popups must be avoided at all times is ignorant; it falls into the "OMG NEVER NEVER NEVER" model which means that you throw out possibly good tools without using a real explanation. There are reasons that popups can be bad. This is one rare instance where popups are good.

2. Why the HTML file for module help? Like I said in #244090: Help text registry, this requires parsing the HTML file rather than just getting the strings from a PHP function.

Two reasons.
1) we have learned that documentors are not developers. They do not necessarily know PHP, and strings embedded into PHP are difficult for them to access. Therefore, help is worked on by much fewer people than it should be, because the strings are not accessible. A primary goal here is to get the help text OUT of the code and into a location where documentors can more freely work on it.

2) By removing the HTML from the code, the help files can be used on other sites without fear that the module will crash or be malicious. The HTML has no code embedded. THis means we can eventually set up a *.drupal.org location that contains help files directly.

3. HTML is being used without putting it in a theme() function. Bad idea.

Help for administrators does not need to be themed.

But it doesn't allow for dynamic help files and on top of that they would have to be translated in a completely different way, inconsistent with normal strings.

Dynamic files can't be indexed by the help system.

I have discussed this with the translation community and this method is preferable because it is very hard to translate help strings in isolation. This allows translators full control over the document. Sometimes the help strings need more than just direct translation to really make sense. This allows the translator full freedom to translate into text that is comfortable for the reader without being tied down to individual strings. Believe me, this excited translators, not upset them.

webchick’s picture

Just to back up the pop-up point. This was almost verbatim what happened with one of the testers during the Usability sprint at University of Minnesota:

"Filter options? What are filter options? 'Click for more information.' Ok! ... Hm. Well this doesn't really make sense to me. Let me go back... Oh... I see I've lost all the data I entered into my form. Darn. Well, what I've learned from this is that asking for help destroys your data. I won't do that anymore!"

Earl, were the discussions you had with translators and others held in one of the g.d.o working groups, or in an issue in the Advanced Help queue, or on the devel list, or? i.e. is there a "More background information about this" link?

Xano’s picture

Two reasons.
1) we have learned that documentors are not developers. They do not necessarily know PHP, and strings embedded into PHP are difficult for them to access. Therefore, help is worked on by much fewer people than it should be, because the strings are not accessible. A primary goal here is to get the help text OUT of the code and into a location where documentors can more freely work on it.

2) By removing the HTML from the code, the help files can be used on other sites without fear that the module will crash or be malicious. The HTML has no code embedded. THis means we can eventually set up a *.drupal.org location that contains help files directly.

Okay, but I'd still like to see a way to create dynamic pages. What about HTML files that may contain PHP like this: <!--<?php echo t('Here\'s some help!');?>-->. The help 'parser' could remove those comment tags and simply parse the file like it would be a template file.

Dynamic files can't be indexed by the help system.

They can, but the dynamic parts may not always be accurate. I would, however, still like to see the possibility of creating dynamic pages. It's not that important that those dynamic parts are indexed with a 100% accuracy.

merlinofchaos’s picture

webchick: There was some talk about this on the devel list when I initially pitched this idea, and in particular Jose Reyero was very enthusiastic about this. Goba also thought this was a generally good idea.

2) By removing the HTML from the code, the help files can be used on other sites without fear that the module will crash or be malicious. The HTML has no code embedded. THis means we can eventually set up a *.drupal.org location that contains help files directly.

Okay, but I'd still like to see a way to create dynamic pages. What about HTML files that may contain PHP like this:

. The help 'parser' could remove those comment tags and simply parse the file like it would be a template file.

Being able to embed PHP at all completely invalidates #2. I understand the desire for dynamic help files but they don't really fit something whose basic concept is that it's basically publishable; dynamic help is difficult to translate and is not accessible to PHP developers. If it is indexed inaccurately this is WORSE than being not indexable.

Xano’s picture

What about not indexing the dynamic parts at all then? Take a look at Dynamic Help (the module) to get a better idea of what I mean before you respond. I am talking about small dynamic snippets that do not necessarily need to be indexed, but that are handy for users to see.

merlinofchaos’s picture

I really feel like what you describe is another category of text entirely, and trying to implement that gets in the way of a lot of very useful functionality that this path provides. Dynamic help could be implemented separately, actually in the PHP module, as you like, but this system could not be.

merlinofchaos’s picture

I wrote up a document explaining the advanced help module and my motivations; I committed it to the advanced help help system.

Now, this document exists for the D6 version that I wrote. This patch, however, is based upon that module, is renamed to help.module, and contains modifications by Gurpartap to completely his Summer of Code. It also is now a little behind the advanced help module, and some of the things I've committed to advanced help very possibly need to be rolled in. All that aside, here is the document:

http://views-help.doc.logrus.com/help/advanced_help/why-advanced-help

Xano’s picture

I can see the use of advanced help, or at least part of it. I got some comments though:

  1. Has access control been implemented?
  2. I keep going on about it: I would really much like to see a solution for including PHP code in help texts. It's not that every module necessarily needs to use it and the PHP code certainly doesn't need to exceed one or two lines to keep the texts maintainable. A few posts back I suggested using <!--<?php echo module_dynamic_help();?>--> for this. It won't be visible when viewing the help pages as HTML files, it doesn't need to be searched, but it's there if needed.
  3. I am still working on a help.module using the menu system (suggested by Robert Douglass) which allows for easy access control and definition of help pages. Although it may not be perfect it would really much like to ask you guys to take a look at it once it's ready. University takes a lot of my time so I'll be done with it this weekend at the earliest. It won't be a fully functional help module, but rather a demonstration of what I described above.
  4. It would be great if we could solve the help in such a way that other help modules may be developed to replace the one in Core. This means all help-related code in other Core modules should be restricted to hooks and other general code every help module may work with. (I dumped the old hook_help() in my demo version and re-used it to solve this 'problem')

Due to lack of time I haven't been able to test Gurpartap's patch, so I might have commented on things that have already been implemented.

webchick’s picture

What is the use case for dynamic PHP help? As far as I can tell, Advanced Help addresses all of the issues I've seen mentioned about our existing help system by the people who actually write Drupal's documentation.

gaele’s picture

merlin, thanks for your explanation of advanced help. I tried advanced help with views, and I really liked it. A big +1 for the pop-ups: you can keep a window on the side while going through the forms. That felt reassuring.

Two questions regarding this patch:

- would it be possible to incorporate images? If so could they be either "translatable" or language independent?
- is access control all or nothing, i.e. all help or no help at all? If so, how and where would help for end-users fit in?

merlinofchaos’s picture

- would it be possible to incorporate images? If so could they be either "translatable" or language independent?

Right now, the help can refer to external items in its own directory, so it can incorporate images. There is a weakness here that still needs to be addressed, in that translators have to copy and/or translate the images. All that needs to happen here is to add an additional way to refer to the original directory separate from the translation directory that translators can use.

- is access control all or nothing, i.e. all help or no help at all? If so, how and where would help for end-users fit in?

Currently it is all or nothing, because the search index has to be filtered to account for those permissions which is complex. I actually had a brainstorm last night and I think I've figured out how to do it, but it would be limited to a per module permission, rather than a per topic permission. Per topic permissions could get very ugly, I think, though they're possible. So this is something that can be added.

I'm not sure this system is really suitable for end user help; I think that using book.module and something else to do popups using the same or a similar system is probably a good choice for end users. That way you could set up the content with nodes and it would be actual site content. Of course, that limits the searchability again, so perhaps not. The system may be extended in the future to do end-user help, but that is not one of its current goals.

Xano’s picture

What is the use case for dynamic PHP help? As far as I can tell, Advanced Help addresses all of the issues I've seen mentioned about our existing help system by the people who actually write Drupal's documentation.

The way Sutharsan and I developed it dynamic PHP help can be used to present the user with a checklist of actions that need to be performed before a module is fully functional. Each action is color-coded (and more) depending on whether it has been performed or not.

I may be wrong, but I'm starting to get the feeling people here don't want to implement the possibility to add PHP just because they don't understand its use, while others keep trying to explain it. I proposed a solution for including PHP in help pages which is both simple and shouldn't conflict with the existing patch. If it does or if you think there's a better way, please tell me I'm wrong and I'd happily accept it. Please stop asking non-specific questions like "What is the use of..?". If you really don't understand something, please be a bit more specific. I don't mean to be harsh, rude or offending, it's just the way I feel about this.

Currently it is all or nothing, because the search index has to be filtered to account for those permissions which is complex. I actually had a brainstorm last night and I think I've figured out how to do it, but it would be limited to a per module permission, rather than a per topic permission. Per topic permissions could get very ugly, I think, though they're possible. So this is something that can be added.

This is one of the advantages my implementation offers. By using the menu system per-page access control is a piece of cake. All help pages are defined as a menu callback in hook_menu(). Help definitions may be overridden by hook_menu_alter() and more than one help text may be assigned to a page if necessary. Modules wouldn't be required to use all of these features, but the implementation as a whole is extremely flexible, simply because the menu system is too.

merlinofchaos’s picture

I may be wrong, but I'm starting to get the feeling people here don't want to implement the possibility to add PHP just because they don't understand its use, while others keep trying to explain it.

And at the same time, I've explained why this is bad and you ignore the arguments and ask why again. Instead, you flagrantly ignore the fact that this is targeted right at people who are not and should not be developers and continue to go on about how you want dynamic PHP, but are unable to address the basic, simple fact that documentation writers will be unable to use this system; or that embedding PHP in the files will make them unsafe for use on *.drupal.org, which IMO is a major benefit.

The more I see your use case, the less I think what you're doing is 'help' and is something that should just be 'module code'. It's something very different, and IMO is aimed at the wrong audience. The big difference I see is that any module can implement PHP code. Because that's what modules are.

I strongly disagree what what you want is a good thing in this system. What you want sounds like a separate system that is highly integrated into the code itself.

JohnAlbin’s picture

Xano, what you describe is a Wizard. Which, while helpful, isn't what the Help system is for. Its for documentation (tutorials, tool tips, readmes, etc.)

You can easily add a Wizard to a module right now. Without any additional APIs. And you can certainly add a link from the help system to your wizard. So I think your use-case is outside the scope of a Help API. If you think the existing APIs are insufficient, perhaps a separate patch for WizardAPI or somesuch thing?

Xano’s picture

And at the same time, I've explained why this is bad and you ignore the arguments and ask why again. Instead, you flagrantly ignore the fact that this is targeted right at people who are not and should not be developers and continue to go on about how you want dynamic PHP, but are unable to address the basic, simple fact that documentation writers will be unable to use this system; or that embedding PHP in the files will make them unsafe for use on *.drupal.org, which IMO is a major benefit.

Do you think every help writer knows HTML? Some will and some will not. After I read your explanation about the HTML files I was very enthusiastic about them and I still am. That's why I came up with the code examples in my last few posts. It allows module maintainers to use PHP the same way it would be used in, let's say, page.tpl.php, but as long as the PHP code isn't executed it's not dangerous at all. Help.module could execute it like *.tpl.php files are executed, but in any other case it's just a simple HTML file. People viewing it won't even see the PHP code, since it's hidden between HTML comment tags (which need to be removed before the PHP code is being executed - _if_ it's being executed, of course). Help writers and translators won't even have to touch it, since every block of PHP should only be one line calling a function that would return the actual dynamic content. Bottom line: I think the occasional short line of PHP code isn't more difficult than using HTML to add structure to your help texts.

Xano, what you describe is a Wizard. Which, while helpful, isn't what the Help system is for. Its for documentation (tutorials, tool tips, readmes, etc.)

It's not, at least not entirely. Dynamic Help (that's a module, please don't mix it up with the term 'dynamic help' which is often used to describe some form of help dynamically generated by PHP) generates a simple list with tasks that need to be done. Wizards are more like point and click setup methods. Sutharsan intended Dynamic Help to be a checklist for users that had problems settings up the module. It's by no means a tool to actually configure the module with.

Aren Cambre’s picture

I would like to lift up Replace included help with links to online documentation (http://drupal.org/node/197771) for discussion.

A model I'm seeing more frequently is that the best and latest help documents are on the product's web site and that local help files, if any, are only used as a fallback.

Microsoft is already doing this with Windows Vista, Visual Studio 2005 and 2008, and Microsoft Office.

This paradigm forces the best and latest documentation to be online and exposed to the public. This has obvious advantages, most importantly that users are guaranteed access to the best and latest copy of the documentation. No more waits for a document push before docs are updated.

Another advantage is document maintenance is as easy as editing a web page. No more messing with CVS, etc. This will make documentation more approachable to many users.

merlinofchaos’s picture

Aren: Please don't hijack this issue. That already has an issue.

alpritt’s picture

1. The issue refers to this being 'Patch #1'. If this is going to be implemented in stages, what will be included in stage 1 and what will be left out until later? It makes it difficult to review if I don't know what to look for.

2. Where am I supposed to put the help directory in that zip file? I'm sure I could work it out from the code, but it would be helpful to not have to hunt for such things.

3. When I click on an inline help link, the help file window opens no problem. However, if I leave the help window open, and proceed to click on another help link, focus does not return back to the help window. It loads fine, but because the window is hidden you could easily think it wasn't working. This is an issue in at least Firefox and Opera on the Mac, however, Safari works as you'd expect. So perhaps this is an issue with the browsers rather than something we can fix. But if we _can_ rectify that, it would be a very good thing.

4. Related to 3. Is it possible to make the back and forward browser buttons appear in the popup window? This would aid navigation.

merlinofchaos’s picture

When I click on an inline help link, the help file window opens no problem. However, if I leave the help window open, and proceed to click on another help link, focus does not return back to the help window. It loads fine, but because the window is hidden you could easily think it wasn't working. This is an issue in at least Firefox and Opera on the Mac, however, Safari works as you'd expect. So perhaps this is an issue with the browsers rather than something we can fix. But if we _can_ rectify that, it would be a very good thing.

This is a browser issue. I don't think it's possible to change focus. Maybe it is, and it just requires a little more javascript.

I'm pretty sure #4 is a no. Gurpartap will have to address #1 and #2.

alpritt’s picture

Using window.focus looks like it covers my point number 3. Point 4 was rhetorical, of course.

edit: on point 4, I believe you can set toolbar to on. You get more than just the back and forth buttons, but I don't think that is an issue. In Firefox you get the address bar whether you want to or not (I believe they force it).

Leeteq’s picture

Subscribing.

Gurpartap Singh’s picture

FileSize
6.8 KB
Failed: Failed to apply patch. View
9.75 KB

The patch #1 contains only the basic structure required to shift to new help system. So that at least, basic documentation can be prepared and committed for core modules by the time all of the features are brought in. Moreover, this would make a bit easy for me to maintain.

What the new module + patch bring in:

  • New .help + *.html files to provide module documentation.
  • Help topics open up in popups, dimensions of which aren't customizable as in advanced help.
  • "Browse help by module" interface on admin/help and on admin/help?popup=1.
  • Fixed template and theme(Garland blue) for popup window.
  • 2 placeholder help files for help.module itself. One is the "About Help" page, and the other is the "How to use help?".
  • A better theme_more_help_link(). Or better to even rename it to theme_help_link().
  • A new function in common.inc: help_exists($module).
  • "access help" permission. Provides full or no access to help topics, popups, etc.
  • Some changes to core to check for "help topics" for automatic "More help" links at admin/by-module and admin/build/modules pages, using help_exists().
  • etc.

What isn't there yet:

  • hook_help() merged into hook_menu(). It's there in my SoC project, however for easier maintenance it's excluded here.
  • Search through help topics.
  • "Welcome to help" isn't there yet. "Browse by module" page has blocks in two columns, like admin page listing help provided by each module. This by-module page will live along with welcome page after it's introduced.
  • Deeper permission system to restrict help topics per module.
  • Tests.
  • Whatever more you can think about, isn't in the package yet.

Other concerns:

  • All the help is provided with the prefix ?q=admin/help/%/% in the url, unlike ?q=admin/help and ?q=help/%/% in adv. help. I didn't want to preoccupy the "help" url. But then admin/help won't make a good sense for a user just checking out filter tips. Any other thoughts on this?
  • Help module behind advanced help module after it's latest 2 commits. I have groked through them, however I'll also need to follow through it's issue to understand the need for those changes.
  • The new module does not override the previous help's expected behavior, viz. hook_help(). But the relation will break as soon as next patches follow.
  • Features to follow in next series of patches. Easier and fun to play with them. They deserve their own space for brainstorming. :D
  • Replace the contents of help.tar.gz with modules/help folder + apply the supplied patch.
  • Why advanced help?
RobLoach’s picture

What is in the tar.gz that's not included in core and the patch? The graphics?

Gurpartap Singh’s picture

It's the complete help module. Patch for the help module didn't make sense because I don't use any existing code from the previous module's code and the new system is completely different. It also does contain a graphic + new files.

Gurpartap Singh’s picture

Status: Needs work » Needs review
alpritt’s picture

Status: Needs review » Needs work

Just a couple of quick things from a very brief review.

1. help_exists() has been declared twice, once in common.inc and once in help.admin.inc. I'm guessing you just want it in the former. If so, perhaps it should be renamed with a drupal prefix (drupal_help_exists()) otherwise it appears like it belongs in the help module itself.

2. In help.js change:

window.open(url, 'help_window', 'width=600,height=550,scrollbars,resizable');

to

      helpWindow = window.open(url, 'help_window', 'width=600,height=550,scrollbars,resizable');
      helpWindow.focus();

which will make the help window popup even if it is already open in the background.

3. Yes, I agree change theme_more_help_link() to theme_help_link().

I'd leave this as review for more comments, but redeclaring help_exists() completely breaks this.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
39.01 KB
Failed: Failed to apply patch. View
  1. Removed help_exists from common.inc, and changed the instances where it was called to module_hook($module, 'help')... Might not be the best solution. Think we should do a help_form_alter instead to change the modules page to add the links? Or move in that help_exists?
  2. Made the JavaScript popup fix
  3. Renamed theme_more_help_link to theme_help_link

Hopefully this patch works because I included the binary files.

alpritt’s picture

Demo site for the benefit of usability folk at http://testing.humte.co.uk/head-help/

Obviously this isn't finished yet, but it should give you a general feel for where it is heading interface wise.

Go to one of the admin sections such as http://testing.humte.co.uk/head-help/admin/content/comment and click the 'more help' link.

catch’s picture

Had a quick go on alpritt's demo site (thanks for setting that up!).

A couple of things I noticed - even if there's no additional help for topic, the 'more help' link still shows up.
Only help/help seems to be working on there too.

The popup's make sense in this situation (despite disliking them in general of course) - and they also don't get caught out by firefox's popup blocker, which is a very good thing.

I noticed "@todo is this trusted output?" - assuming all help files are provided by modules, then we should probably assume they are. Leaving things open for people to embed flash videos etc. would be a very nice thing here etc.

There's some minor code style issues (code comments, lower case CSS hexes). And of course only having help for help makes it hard to review the /help index side of things.

In regard to admin/help and/or help - can we just make everything /help and move this out of /admin?

Probably more questions but that'll do for now. Nice work :)

moshe weitzman’s picture

I am not seeing any help topics in the demo site.

gaele’s picture

I guess Patch #2 would include:
- renaming "More help" to "Help", and
- moving introduction texts like the one on /admin/content/taxonomy completely to the help pages

yoroy’s picture

screengrab from the demo site:

It would be great to get this basic functionality in. If we have this ability to "just put a help link" like this, we can move on and build a better welcome page. As you know, the disappearing welcome page confuses a lot of people that are new to Drupal. We need this humble help link in place before we can build a better transition from succesful installation to actually getting started doing your thing with Drupal.

So from this point of view "just getting the pop-up help link in place" should be the focus of round #1 for this patch. 'Help' should ideally only be a last resort, but this needs fixing before we can work on lowering the need for help at all!

Xano’s picture

Perhaps we could allow modules to specify a handbook page where more documentation about those modules can be found. I remember reading something about a hook_config() for D7? Perhaps we could declare the URL there. The link to such a handbook page can be displayed along with the links to modules' permission settings and administration pages.

Gurpartap Singh’s picture

FileSize
6.64 KB
Failed: Failed to apply patch. View
15.46 KB

Updated help-rtl.css.
And &path& instead of path:, etc. as per suggestion here: http://drupal.org/node/299050#comment-978226

Gurpartap Singh’s picture

Also updated "More help" link to only show up when help actually exists (using help_exists()).

catch’s picture

Category: feature » task
Priority: Normal » Critical
Status: Needs review » Needs work

Gurpartap - please post everything as a single patch (minus the binary files if that's tricky) - it's much easier to do a code review if I can just open the patch file in the browser (and apply a patch from Drupal root without extracting a zip) - and I know that's the same for others who might review this patch too. Also, correct status ;)

beejeebus’s picture

subscribing.

chx’s picture

A lone return is never necessary, please remove that and the else{} with it. Instead of drupal_function_exists('help_get_topic'); $info = help_get_topic($module, $topic); you can easily use module_invoke('help', 'get_topic', $module, $topic).

Xano’s picture

Something I just thought of: when putting all help into external files the markup may not fit in with the active theme, since theme overrides aren't applied to it.

mgifford’s picture

Quite looking forward to having help pages available to those people who are most likely to need them (and indeed in a manner that they are likely going to want them). The idea of tying the help pages to watchdog pages is just baffling. access administration pages is too general.

Also, in searching on this topic, wanted to point folks to the usability group thread on this:
http://groups.drupal.org/node/6297

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
15.58 KB
6.58 KB
Failed: Failed to apply patch. View

In sync with D7 api changes and some of advanced help changes:

- Different name for a module on help listing and on help topic title and breadcrumb. e.g. Content module can name itself as CCK.
- "hide" setting to let modules not show up themselves on help listing.
- Restore popup window focus when clicking popup link (while the previously opened window is still open and hiding somewhere on desktop ;).
- "Up" navigation as in book module.
- use drupal_alter so that topics and the topic info cache can be modified by modules.
- bug fixes.

(Refer to http://drupal.org/project/cvs/248489 for specific issues.)

Gurpartap Singh’s picture

catch forgive me for my inability to get the changes included in a single patch. I still haven't been able to discover how can one include a new folder with a new file in a cvs diff patch. Anyhow, awaiting reviews.

dmitrig01’s picture

FileSize
6.64 KB
Passed: 10551 passes, 0 fails, 0 exceptions View

Re-rolled so that the patch works.

keith.smith’s picture

Status: Needs review » Needs work

On an initial read-through of the patch, there are a few minor code style issues. I'll give this a functional review soon.

+ * TRUE if the help module is installed & enabled and the given module provides help topics.

Use "and" instead of "&" in code comments.

+      // Set a class for links to be opened in a popup; degrades in the absence of JS.
+      // Include the javascript for launching popup for hyperlinks with class 'help-link-popup'.

Use "JavaScript" in code comments, rather than JS or javascript.

+ // Return empty is the specified topic doesn't exist.

is = if

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
6.63 KB
Failed: 9665 passes, 11 fails, 0 exceptions View

As a note, there's a & in drupal_to_js()'s comment too.

tstoeckler’s picture

I wanted to propose to make the help links next to the modules on the admin/build/modules page to appear not as "More help", but as "More info". You need 'help' if you have a problem, if something isn't working the way you wanted it to, etc. The link next to one of the modules is used in a different situation:
1. The user wants to choose which modules to install,
2. He/She has read the module's name and doesn't know whether he/she needs it or not.
3. He/She has read the module's description and still doesn't know whether he/she needs it or not.
In this case the user doesn't need help, but additional information about the module, which is exactly what the help page gives him.

Just wanted to put that up for discussion.

catch’s picture

Status: Needs review » Needs work

This patch really needs at least one core module other than help.module) converted to use the new help files to make the functionality easier to review - then we can see what the differences are with hook_help() a lot easier, will be easier to take screenshots etc.

Pasqualle’s picture

subscribe

keith.smith’s picture

I've installed the patch and associated modules/help/* files; I thought it might be helpful to convert some additional core help pages to the new system so that there's simply more help pages to try out. I'm working on figuring out how to do so now.

If the existing module help pages show up as "About $foo", then I'm hoping this allows us to accommodate additional entries in the help text index. It'd be nice to include some topic-based help like "How do I add a user?" or "How do I add content?" I'm not positive how to do this yet either, but will see if I can figure it out.

One small thing -- on first blush, with only the included help module help installed, the "Up" navigation link at the bottom of the page doesn't work; on my system it links to admin/help/help (the same page), rather than the admin/help page like I expected.

Edit: Also, in the modules/help/help directory, both help.help and how-to.html exist but are empty. Is that normal?

Edit: I see. If I copy in a reference in help.help similar to the foo.help.ini file in the advanced_help checkout, it works fine. Those files have gotten hosed in the latest zip file in the issue though.

keith.smith’s picture

Now that I have the additional help files (from the advanced_help checkout) in my patched copy of HEAD, it appears that the "Up" navigation link sometimes works correctly. Specifically, when viewing admin/help/help/using-advanced-help, for instance, "Up" takes me to admin/help/help. "Up" on admin/help/help, though, takes me to admin/help/help rather than admin/help as expected.

keith.smith’s picture

Also, I should say that although this patch is hard to review in its current form (since it requires a patch, a zip file, plus a few extra help files from advanced help), after getting it working, I'm completely sold. I really didn't expect to like this, but I was completely wrong; this is absolutely the way we need to go.

I'm very excited about:

  • the ability of modules to provide multiple help entries (not just the one), finally giving us a platform for task-based help.
  • the ability of modules to provide entries in another module's index. If we wanted, a task-based bit of help on posting content via the BlogAPI (and provided by modules/blogapi/help/how-to-use-blogapi.html) could appear under the index for node or blog or what-have-you, instead of under BlogAPI where it's only going to be found by someone who already understands what BlogAPI does.
  • additional navigation, search (though it doesn't appear that search is implemented in the core patch), and popup tools bringing a fresh face to an overlooked area of Drupal core.
  • editing (and creating!) help text by modifying a simple HTML snippet, or at worst, an HTML snippet and a .help (ini) file, rather than working with php code.
  • being able to use the right tool for the right job. HTML was already our best bet for displaying rich help, with nice formatting, images and icons. This patch leverages what HTML already gives us.

I'm volunteering to (a) convert the existing module help entries, (b) write up a first draft of some new topic-based help texts, and (c) work on some new standards and tutorials that fully document this new style of help generation. Obviously, at least (b) and (c) should happen in different issues and will be dependent on this making it into 7.

keith.smith’s picture

FileSize
2.74 KB

Here's the about.html file for system module, which would need to be placed in a new directory named /modules/system/help.

keith.smith’s picture

Since this is difficult to review from the patch, I made a quick demo site at http://helpdemo.advantixllc.com. It features the files from the patch, plus some additional help added from the advanced help module itself and a new help file I made for system module. I'll work on doing the other modules (and maybe adding some task based help) over the next several days.

Note that on the demo, I've given anonymous users access to view help (also note that the patch's implementation of help_perm is out of date, and doesn't include both a title and description).

keith.smith’s picture

Just so I don't forget, the patched help module needs its .info file tweaked to reflect the "Core - optional" to "Core" change.

Gurpartap Singh’s picture

@keith.smith Did you take a look at the new http://groups.drupal.org/help-system setup for documentation task? I have added your Aggregator and System help over there. The description over there reads: ".. to collaboratively write a comprehensive documentation of the features provided by the modules in Drupal 7 core.", in which topics like "Adding users", "Adding content" are included! Advanced help's help topics provide more information about what you need to put in .help files for adding more topics to a module.

redndahead’s picture

I have some fairly comprehensive help documentation in the advanced_help format that I can probably contribute. Some screenshots will need to be replaced because it assumes the usage of admin menu and it is not using a base theme. I'll see if I can group these together and add them to the groups page.

Blocks
How to move blocks

Blogs
Add Blog Item
Delete Blog items
Edit blog item
Introduction to blogs
Enable blogs menu item

Books
Add book
Add book child
Arrange book pages
Delete book pages
Edit Book
Enable book menu item

Contact
Add Contact Category
Edit Contact Category
Delete Contact Category
Add Additional Contact Information
Enable contact menu item.
Contact Introduction

Forum
Add Container
Add Forum
Add Topic
Arrange Forums and Containers
Delete Forums and Containers
Edit Forum and Containers

Menu
Move menu items
Add a menu item to an external link.

Modules
Enable
Disable

Pages
Add
Edit
Delete
Delete Multiple Pages

Polls
Add
Edit
Delete
View Results
Enable Block
Enable Menu Item

redndahead’s picture

FileSize
142.84 KB
212.69 KB
602.17 KB
66.25 KB
226.08 KB
265.33 KB
207.81 KB
189.21 KB
77.05 KB
50.33 KB

Can't attach tar.gz files to groups, so I'll attach them here for future reference.

Gurpartap Singh’s picture

Tremendous job redndahead! It seems like a help suite for a client's website, since I read some unfamiliar terms in user help. I'll be moving a few of these to the group documentation today! (For anyone who wants to test these with D7: these are missing the .help files.)

keith.smith’s picture

Fantastic, redndahead! I'll take a look at these in a bit.

@Gurpartap Singh: Thanks for pointing me to that groups page; I hadn't seen that in a long time and it was very helpful. Question though -- at least in regard to the core help texts (like the aggregator and system help I did last night), we're just converting what's currently in core to the new format, right? I'd think it'd be well out of scope to add the new help system AND change the verbiage of existing help text in the same issue? Also, I'm not yet sure what the best way of distributing this patch is so that it'll be easy to review; as you mention, using fakeadd or whatever to add files to patches is a pain but is likely the only option. It'll be a big patch considering that it'll touch every module in core (adding new help directories and removing part of the hook_help implementation).

catch’s picture

I think the ideal thing here would be a patch with just a couple of core help texts converted (with no textual changes) - so there's something to review but not too much directory adding to maintain for re-rolls. Then we can ask webchick or Dries to create the help directories in each core module on the first commit - making it a lot easier to convert the rest in followup patches.

yoroy’s picture

Just chiming in to say I'm happy to see this moving forward. And what catch says. Rock on!

Gurpartap Singh’s picture

@keith.smith I am looking forward for a refreshed, and user-documentation like help in core, since the bare minimum "descriptions" provided by the current help data don't help a lot; and lack of better help would put this patch to shame. :)

I'll see how fakeadd can help with creating a diff for new files under a new directory.

@catch Sure, that's how I have decided as well. I just took the opportunity to promote the documentation task.

webchick’s picture

Just a quick note about the suckiness of CVS, since I can never miss an opportunity to rant about how much I hate it. CVS won't let you add empty directories. :( So catch's otherwise brilliant plan in #78 won't work.

If it's possible to convert them all with little headaches, that would be ideal, so they can all go in at once. If that's a huge PITA, then no worries -- we can do it gradually.

keith.smith’s picture

FileSize
45.34 KB
Failed: Failed to apply patch. View

Attached is a consolidated patch -- except for the about.html files in the individual module directories, and possibly gradient.png in the modules/help directory.

I believe everything else should be in here. I also fixed the tag in the help.info file and the updated help.module to the new hook_perm format (adding both title and description). Unfortunately, there's a good number of Help system test failures that I haven't looked at yet, so I'm leaving this at CNW. Also, I have not eliminated the help page areas of each core module's hook_help implementation (that would make this patch even larger and harder to review). Perhaps we could do that as a followup patch to avoid that.

I'll tar up the individual core module about.html files and attach them to this or another post in a few minutes.

keith.smith’s picture

FileSize
545 bytes

Also, this script file can make those help directories if its run from the core modules directory.

keith.smith’s picture

FileSize
26.37 KB

Attached is a .tar.gz file with a directory structure and about files for all core modules, replicating AFAICT the existing help in those modules. It's not exactly as simple as a cut and paste, but mostly is -- the links have to be changed to include a &base_url& as appropriate. I also added a <!-- $Id $ --> line to each file.

Thanks to all the people (redndahead and Gurpartap Singh, mostly, who did the majority of this work through posting the cut-and-pasted pages to various g.do. pages.

keith.smith’s picture

Status: Needs work » Needs review
FileSize
26.37 KB
224 bytes
47.66 KB
Failed: Failed to apply patch. View

This new version of the patch in #83 clears up the test failures for me. I'm also re-attaching the tar of the about.html files from #85, though I haven't changed them, and a gradient.png file while should live in /modules/help. I'm setting to CNR to get a testbot review.

I'm still think there's an issue with the book-like navigation, as noted in #68.

Status: Needs review » Needs work

The last submitted patch failed testing.

keith.smith’s picture

Yeah, what was I thinking. The testing bot is going to return a failure because all of the files in #86 need to be applied, including the about.html files that live in the new directories.

Edit: I should also mention that the demo site http://helpdemo.advantixllc.com/ shows the about.html files as they are in the tar file.

redndahead’s picture

We will need to add the module.help.ini files to these. I would also like to see us make room for images. I was thinking one of two ways could work when there is multiple help files for the same module.

modulename/help/modulename.help.ini
modulename/help/helpfile.html
modulename/help/images/helpfile/image1.png

or

modulename/help/modulename.help.ini
modulename/help/helpfile/helpfile.html
modulename/help/helpfile/image1.png

I think the second one is the better layout.

What y'all think?

keith.smith’s picture

But the foo.help (not ini) files are only necessary when there are multiple files per topic (only help has multiple files in this iteration). About.html is a magic name that appears if it is present, without the .help file.

Hopefully, we will have multiple files per topic so as to allow for a topic based help index. But, I bet we won't get away with adding non-core help in this issue -- I think we'll need a new issue to add new help after this one goes in, and that issue can add the foo.help files.

Or do I misunderstand?

redndahead’s picture

You are correct I didn't realize there was a special section for about. I thought this was just a straight port of the advanced_help module. I don't know if I'm a big fan of this. I realize that this makes it easier to add a help file and it's probably used in special places, but I'm not sure that it's that hard to add the additional modulename.help.ini file. I think if we need to designate a special file I would rather see it designated in the modulename.help.ini file. Something like Default = TRUE

Either way I think we should still add the modulename.help.ini files to this so future patches that include more help files don't require us to add this file. It wouldn't hurt anything.

keith.smith’s picture

There's not really a special section for "About" (though I think maybe there should be), but about.html doesn't have to be in foo.help for it to appear. I'm on the fence about this as well. It's nice in that it let's you add the 'About foo' page easily, but I'm not convinced that the 'About foo' page should be treated differently (including its special weighting). It'd almost be a more consistent DX to have to add all files in the foo.help file.

There is no foo.help.ini AFAIK. In this version, it's foo.help.

redndahead’s picture

hmm time to really read through this patch. Seems there are quite a few main differences between advanced_help and this patch. I promised to do a review earlier so I guess now is a good time.

catch’s picture

I'm also not convinced by the magic about.html file - we specify everything in .info files for modules, help should be the same IMO.

merlinofchaos’s picture

Note that placing help images outside of the help directory might complicate life for translators, who have the option of translating images. (Note that there was a bug fix for that in advanced help that I am not sure if it has made it into the most current patch here. I know Gurpartap does port fixes sometime after I make them but I lose track)

alexanderpas’s picture

I'm against using &tokens& we should use [tokens] so they can be handled by core later on.

also, some t() code in the testcases is very ugly.
I (semi-)don't like the output documentation of hook_help regarding t()

redndahead’s picture

The main reason for using &tokens& was so we can document them in the help files. There is no html entity for [ or ] so when you document it, it get's replaced.

alexanderpas’s picture

understandable, however you can always use the HTML Entity Number. &#91; and &#93; for [ and ] repectively

redndahead’s picture

Sure enough. It seems reasonable. The only thing I'm concerned with is for some things it doesn't act like a token. For example &topic:module/topic& As much as that is a replacement I don't think this can be handled by tokens as is. I could be wrong though.

alexanderpas’s picture

first implementationin core and current tokens won't support that, however, later implementations may support that type (if properly registered).
also, if a token is not found, it won't be replaced, so you can handle is anyway.
might i suggest [help:module/topic] ;)
you might want to check #113614: Add centralized token/placeholder substitution to core

keith.smith’s picture

FileSize
43.01 KB

This is a new tar.gz file of the about.html files in each core module's help directory. In the previous version of this file, I had left out filter module and used the wrong style id tags at the top of each file.

yoroy’s picture

Maybe someone could summarize the remaining to-do's and issues we have to work on? Would love to see another burst of progess here!

What I can do is post a screengrab of the demo in #88, is this still how things look at the moment?

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
224 bytes
45.63 KB
Failed: 8549 passes, 18 fails, 0 exceptions View

Now includes help.test testing popup and normal help page results. I seriously hope we are near to RTBC or RTBTC! :P

gradient.png needs to go into modules/help folder. and the help_files.zip to be extracted in drupal root (contains intro articles on color and help module).

Michelle’s picture

My son just woke so I wasn't able to get too far into this. Here's the notes I took so far. I'll look some more when I get more computer time. This is my first time ever attempting this and I didn't read the issue as per webchick's instructions, so sorry if some of this is silly...

"TRUE if the given module is enabled, provides help topics and the help module is enabled." <-- What does this mean?

-----

+ drupal_function_exists('help_get_topic');
+ $info = help_get_topic($module, $topic);

If the function doesn't exist, what happens here?

-----

theme_help_link has a lot of logic and just a little HTML. If a themer wants to change that HTML, they have to copy all that logic into their override, correct? Could the logic be done in a preprocess instead?

-----

help-popup.css

I just read somewhere that the css bits should be in alphabetical order. Not sure if that's an official standard or not.

Michelle

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

IMO there is too much logic in theme_help_link(). Example:

if (isset($topic) && !$info) {
  // Return if the explicitly specified topic doesn't exist.
  return;
}

theme_help_link() shouldn't be called at all if there is no topic. Theming functions should be used for theming only and not for logic. Drupal is hard enough for themers already.

Also, HTML is being used outside theme functions, like in help_view_topic().

By the way: have you succeeded in implementing per page access control?

Gurpartap Singh’s picture

Status: Needs work » Needs review

+ drupal_function_exists('help_get_topic');
+ $info = help_get_topic($module, $topic);

If the function doesn't exist, what happens here?

It's actually a method to include() the file that contains the function in the arg. I should comment on that.

theme_help_link has a lot of logic and just a little HTML. If a themer wants to change that HTML, they have to copy all that logic into their override, correct? Could the logic be done in a preprocess instead?

Possible, but I'm not sure if it's good practice to use preprocess upon the theme function of same module.

help-popup.css

I just read somewhere that the css bits should be in alphabetical order. Not sure if that's an official standard or not.

Sorting the whole CSS file would be horrible, but sorting with some granularity maintained for the selectors would be fine, rather good. I'll try to update the patch upon more feedback, and bot testing. THANKS!!!

"TRUE if the given module is enabled, provides help topics and the help module is enabled." <-- What does this mean?

Nonsense.

Michelle’s picture

I only have a quick minute but, to clarify the alphabetising is like "background" before "color" in each section, not alpha the whole thing.

Michelle

merlinofchaos’s picture

Gurpartap: Here are my review comments:

I'm uncomfortable with 'More help' being untranslated and then t($title) later -- that's more work that needs to be done by the extractor. Is there a good reason for this?

Return code that emits a 'more help' link to view a topic in a popup.

Since the word 'more' was removed from the theme function it should be removed from this comment.
Possibly also from the link title.

I see the help icon was removed. I think that is needed, though my original implementation is inflexible. However, an icon can be added back in later, so I think I'm ok with this.

Why not put theme_help_link in help.module? The theme system will do nothing if it can't find a theme implementaiton to use, so there is NO cost to theme('help_link') if the module doesn't exist. As it is now there actually IS a cost because it goes through the entire theme system just to print no output.

+ drupal_add_css(drupal_get_path('module', 'system') . '/admin.css');
I don't remember why we're doing this. Why are we doing this? Should be commented and explained. Also does it need to be system's admin.css?

The declaration for help_topic_page has $topic = NULL but the menu declaration will always send TWO arguments; there is no point in a default here and it suggests that a topic may not be given.

For the drupal coding style, $pmodule should probably be $parent_module

+ // Loop checker.
Something more descriptive, like:
// Crawl up parent tree looking for the breadcrumb trail.

Then the loop checker comment should go to the point that actually checks for looping.

help_view_topic() has almost no comments for building the navigation section. We could add some here.

+ elseif ($_GET['q'] == "admin/help/$module" || $_GET['q'] == "admin/help/$module/about") {
Is there something we can test other than 'q' for this?

I feel like help_get_topic() should be help_get_topic_info() or something along those lines.

I'm really uncomfortable with the code for _help_check_default_file(). It's checking filenames directly and it's looking directly at help_example with no explanation. What's going on here?

"Sort topics information array." should be "Sort topic information array."
A bit more description of how we're sorting would be useful, though it is just weight/title so that might not really be necessary.

"Load and return help topic info." should be more like "Return information about the help topic file" and more description: "This function checks a list of possible locations for a help topic file, allowing translations and the current theme to override the default location of the file."

+function help_get_module_name($module) {
This should be moved to module.inc or something, this is not really help specific.

theme_help_by_module looks like a candidate to be split into a preprocess and a template.

When viewing /admin/help the bullets are hanging out the left of the gradient boxes.

That gradient seems to be used very commonly. Perhaps it shouldn't be with help specifically? Especially since it's Garland specific. Garland should be the one to include the gradient in its style.css (and likewise Minelli I suspect).

merlinofchaos’s picture

Additionally, the documentation for help itself should be included, in particular the .ini file documentation. Without that documentation it's really difficult to tell what the .ini file is doing. Please include that with this patch.

merlinofchaos’s picture

You can't use a preprocess on a theme function, only on templates (for performance reasons).

It's not clear to me that the help link thing actually is a themable; in advanced help, I had made it themeable to make it safe for modules to just include theme('advanced_help_topic') without worrying about whether or not advanced help existed. It was a bit of a cheat to use one system for another thing. We could just as easily include the function in common.inc and have it do the check and do all the logic. The link itself has only a tiny bit of HTML, so if there is a themable there it could be very small.

Xano’s picture

Possible, but I'm not sure if it's good practice to use preprocess upon the theme function of same module.

I often hear people that want to theme, but who aren't very good at PHP complain that the chunks of PHP in some theme functions are overwhelming. My suggestion to remove as much logic from those functions was made from that point of view.

By the way: have you succeeded in implementing per page access control?

Michelle’s picture

Found the link for my CSS comment: http://drupal.org/node/302199

Michelle

keith.smith’s picture

The file in #101 should include the existing help files for all core modules, in an advanced help format. It also includes the "How to use help" related files that ship with advanced_help, that merlinofchaos references in #110. Existing core module help needs to go in with this patch, IMO, though perhaps we could wait until a followup patch to strip the embedded version out of the core modules.

Gurpartap Singh’s picture

FileSize
224 bytes
15.74 KB
45.44 KB
Failed: 8549 passes, 18 fails, 0 exceptions View

@merlinofchaos

I'm uncomfortable with 'More help' being untranslated and then t($title) later -- that's more work that needs to be done by the extractor. Is there a good reason for this?

Return code that emits a 'more help' link to view a topic in a popup.

Since the word 'more' was removed from the theme function it should be removed from this comment.
Possibly also from the link title.

Fixed. t("More help") is set if $title is NULL. "More help" is likely to be used a lot throughout admin pages, hence its retention.

I see the help icon was removed. I think that is needed, though my original implementation is inflexible. However, an icon can be added back in later, so I think I'm ok with this.

Icon's still there for ".more-help-link a" in system.css

Why not put theme_help_link in help.module? The theme system will do nothing if it can't find a theme implementaiton to use, so there is NO cost to theme('help_link') if the module doesn't exist. As it is now there actually IS a cost because it goes through the entire theme system just to print no output.

Fixed.

+ drupal_add_css(drupal_get_path('module', 'system') . '/admin.css');
I don't remember why we're doing this. Why are we doing this? Should be commented and explained. Also does it need to be system's admin.css?

We don't need this, in fact. Fixed.

The declaration for help_topic_page has $topic = NULL but the menu declaration will always send TWO arguments; there is no point in a default here and it suggests that a topic may not be given.

Since I find no way to combine these two in help_menu():

  $items['admin/help/%'] = array(
    'page callback' => 'help_topic_page',
    'page arguments' => array(2),
    'access arguments' => array('access help'),
    'type' => MENU_CALLBACK,
  );
  $items['admin/help/%/%'] = array(
    'page callback' => 'help_topic_page',
    'page arguments' => array(2, 3),
    'access arguments' => array('access help'),
    'type' => MENU_CALLBACK,
  );

I have to set $title to default to NULL for the first menu entry.

For the drupal coding style, $pmodule should probably be $parent_module

Fixed. (for $ptopic too)

+ // Loop checker.
Something more descriptive, like:
// Crawl up parent tree looking for the breadcrumb trail.

Then the loop checker comment should go to the point that actually checks for looping.

help_view_topic() has almost no comments for building the navigation section. We could add some here.

A few comments added, explaining the tasks being done.

+ elseif ($_GET['q'] == "admin/help/$module" || $_GET['q'] == "admin/help/$module/about") {
Is there something we can test other than 'q' for this?

Now using:

  elseif ($topic == 'about') {
    // Link to the main help page, if we are on the about page.
    $up = 'admin/help';
  }
I feel like help_get_topic() should be help_get_topic_info() or something along those lines.

Changed to help_get_topic_info().

I'm really uncomfortable with the code for _help_check_default_file(). It's checking filenames directly and it's looking directly at help_example with no explanation. What's going on here?

AHH, swept it. :P

I can't think of any other way to arrange for the "default file". (Isn't it a good idea, since sometimes devs can be too lazy to write a .help file).

"Sort topics information array." should be "Sort topic information array."
A bit more description of how we're sorting would be useful, though it is just weight/title so that might not really be necessary.

"Load and return help topic info." should be more like "Return information about the help topic file" and more description: "This function checks a list of possible locations for a help topic file, allowing translations and the current theme to override the default location of the file."

Fixed.

+function help_get_module_name($module) {
This should be moved to module.inc or something, this is not really help specific.

I don't think so, since module's name can be modified using "help settings" key.

theme_help_by_module looks like a candidate to be split into a preprocess and a template.

It was a copy of theme_system_admin_by_module() and the same has now been put into use.

When viewing /admin/help the bullets are hanging out the left of the gradient boxes.

I'm not sure how this has changed since the last time we had patch reviewing. I find left hanging elements even on admin/by-module page. system.css annotate doesn't reveal anything good to me. Anyways, so I need to specify the styling for them. (TODO)

That gradient seems to be used very commonly. Perhaps it shouldn't be with help specifically? Especially since it's Garland specific. Garland should be the one to include the gradient in its style.css (and likewise Minelli I suspect).

It's just that it presents the thing looking well in-line with the rest of the "base" drupal theme, viz minelli/garland. The gradient in garland is 180deg rotated, which doesn't fit here. In case this isn't pleasant(i highly doubt, look at the screen shots above), we can get rid of it.

Additionally, the documentation for help itself should be included, in particular the .ini file documentation. Without that documentation it's really difficult to tell what the .ini file is doing. Please include that with this patch.

Sure. :-)

You can't use a preprocess on a theme function, only on templates (for performance reasons).

It's not clear to me that the help link thing actually is a themable; in advanced help, I had made it themeable to make it safe for modules to just include theme('advanced_help_topic') without worrying about whether or not advanced help existed. It was a bit of a cheat to use one system for another thing. We could just as easily include the function in common.inc and have it do the check and do all the logic. The link itself has only a tiny bit of HTML, so if there is a themable there it could be very small.

Are we splitting the logic and theme? Then, would I be calling the logic in the theme function? Or some other way you are thinking about? (TODO)

EDIT: THANKKKKKKKKKKKKKKKSSSSSSSSSSSSSSS!!!!!!!!! :-)

(reviewing info: gradient.png needs to go into modules/help folder. and the help_files.zip to be extracted in drupal root (contains articles on help and color module.))

Gurpartap Singh’s picture

Xano: By the way: have you succeeded in implementing per page access control?

Honestly, I haven't looked deep into it yet, although I would, and other too, feel the necessity one day.

Michelle: Found the link for my CSS comment: http://drupal.org/node/302199

Fixed in patch above. :)

Gurpartap Singh’s picture

System Message: The last submitted patch failed testing.

help.test successfully passes. it's rather forum and couple of other modules generating errors causing the patch failed message.

Xano’s picture

1) Are the help pages also browsable by hitting "Help" in the menu and simply browse the pages?
2) How do modules tell Drupal what help pages they offer?

The code example in #115 only shows callbacks, not normal menu items. I still think Robert Douglass' (and my) original idea of using the menu to define help pages is a good idea. This would open up the possibility to browse through help using a drop-down menu (It's a possibility, it doesn't necessarily have to be default behaviour) and it would make per-topic access control a piece of cake to build in. The previously mentioned code in #115 makes me think this wouldn't be very hard to achieve, would it?

Status: Needs review » Needs work

The last submitted patch failed testing.

merlinofchaos’s picture

Status: Needs work » Needs review

Xano: The pages are browsable. This is one of the reasons I want it to include the help documentation, so it can describe the creation of the help files right in this patch. I don't believe using the menu system for help is a good idea, that puts too much burden on development and the idea here is to separate help text as completely as possible from development.

Xano’s picture

How do modules tell Drupal what help pages they offer?

You haven't answered this question ;-)

I don't believe using the menu system for help is a good idea, that puts too much burden on development and the idea here is to separate help text as completely as possible from development.

What about automatically creating a menu item for every help page? I understand that new help pages can now be added without having to write PHP? Drupal somehow knows about those pages and it also knows its hierarchy, which should be stored somewhere. At that same place a permission could be stored. When indexing all the help pages menu items can be generated using this permission.

Gurpartap Singh’s picture

How do modules tell Drupal what help pages they offer?

They are defined in module_name.help file. See the help ini info page in hep_files.zip.

Regarding the using menu system, with 50(more or less) modules enabled, with each having at least one(possibly more than one) help page, would be pure chaos in menu.

merlinofchaos’s picture

As an example, I've attached the ini file and html that came with my original advanced help module. I believe a couple of minor things have changed with these in the patch so these are not up to date, but Gurpartap should hopefully be posting a version of the patch that includes properly updated versions of these.

Gurpartap Singh’s picture

I had actually included those in the help_files.zip in #115, although I mistakenly skipped editing them to change "Advanced help" references to normal Help.

merlinofchaos’s picture

It's just that it presents the thing looking well in-line with the rest of the "base" drupal theme, viz minelli/garland. The gradient in garland is 180deg rotated, which doesn't fit here. In case this isn't pleasant(i highly doubt, look at the screen shots above), we can get rid of it.

I still feel this is theme and should be moved into garland (and minelli). It will look awful in, say, bluemarine. Themes are welcome to add their own spiffy backgrounds to this page. Many themes customize the top level administration page, the same will be true for the help page.

merlinofchaos’s picture

Looked at the files in #115. They need to be updated for the current system (no references to advanced help, for example, change to the proper filenames actually used. Make sure all the features talked about are correctly named).

The problem with the bullets is that Garland is imposing very small margins on the ul -- I'm not quite sure the best way to do this.

Now that we have more items in for the help module text, I really love the toc block. That's good stuff right there.

One minor problem I'm having with the navigation: All modules are having their top level TOC included on the help page, but the breadcrumb has the 'about' page as the page for the module. When visiting the 'about' page you thus have a breadcrumb link that points to where you are; it makes it feel like the about page is the parent of other pages, but the navigation isn't reflecting that. It feels inconsistent. Maybe the about page needs to be promoted a bit so that it's automatically the parent which will make the navigation feel a little smoother? I'm not quite sure what the right answer is here but it's definitely a hitch.

merlinofchaos’s picture

As I think on this, I think we need to re-think the way that works. We have a help page which represents 'the module' which is currently being automatically extracted as the about page. We're also displaying the TOC for the module on the help page in a box; now, I have a fear that with this fully fleshed out, we're going to make that *really* big. Each module's box on the main page should be much more limited. What if we went this route:

  1. Put a short description in the .ini file itself. That would appear as the "description" text in the block on the help page. I think right now we're using the module description but that may not be accurate, especially if we're using modules to include help books, which I think is a long term goal here.
  2. We limit the help topics listed on this page somehow. We either a) pick the first 5 top level items and display those, no more. Or b) we create an item in the .ini file to pick up to 5 items and display those so this can be completely customized to include the 5 most important help files right there. Either way, we then have a 'more...' link which goes to the top level help page for that module, thus breaking into the normal TOC navigation as well as the descriptive text.

Thoughts on that?

merlinofchaos’s picture

One last comment: I'm not in favor of the automated 'about...' text. Let's create a guideline, make all of core stick to that guideline, and let contrib modules implement as they fit. We can create a special pointer in the .ini file to serve the purpose of the about page now, but right now the automation feels restrictive and non-intuitive. The about doesn't appear in the .ini file, which means a casual developer scanning this cannot easily determine what's going on. (This is a setup we have in Drupal right now which I have decided is bad; too much happening automatically with no way to tell what's happening by looking at configuration). This makes the name 'about' too much like magic.

mfer’s picture

I hate this... subscribe.

add1sun’s picture

OK, I have just applied the patch and played around with it a bit - I haven't actually reviewed the patch yet. In terms of just using it and not really knowing much about how it all works "under the hood" here are some observations and questions:

1) As noted earlier the example help files need to be updated. The first thing I did to see what was going on was to open those up and when I saw that they weren't accurate (.ini filename, referring to advanced help) I assumed they were "gibberish" examples rather than real examples that explain how this works. I stuck with it though and realized they are largely accurate - but I have no was of knowing that without actually poking into it and figuring it out. I also couldn't figure out where the about.html page was coming from. The color.module example points to one in the .help file but the help.module example doesn't. If we want more people to review and understand, getting that info accurate is important.

2) Like merlinofchaos noted, the About page is the default, top level but not really reflected in the menu structure. It also doesn't have an active class unless I actually click the link for the about page in the menu. Then it gets its active item coloring. Not horrible but a little disorienting at first about "where I am."

3) The auto-alphabetizing of topics seems odd. There is rarely a case that alpha is the correct order for pages. I'd simply assume that the order they are in my .help file was the order they would be displayed. Other than being the expected behavior (from my perspective, I know) it would also do away with weights, which all around is a good thing. Weights are the devil's own and tend to cause more confusion than help.

4) Why do we need the funky syntax stuff in the html for links/images and is there a reason we need that *specific* syntax ie. the & versus some other token-like symbol)? I'm sure there is a good reason, but I haven't researched at all. It frankly looks kinda weird and confusing for some reason and I wonder, depending on the whys if we can use something that visual looks cleaner. Then again, maybe that's just me. For those just reading along , this is what I am referring to:

<a href="&topic:module/topic&">
<a href="&base_url&admin/settings/site-configuration">

I'll be reviewing the patch itself and getting more time with this whole thing this week.

Xano’s picture

Could a callback token also be introduced to allow modules to insert something into the page? Just to keep a feature that has been present in Help.module for the last few versions. I was thinking of something like &callback:contrib_function&. Help.module could replace this with the return value of contrib_function().

webchick’s picture

Yay, glad to see this patch get some eyeballs from the docs team. :)

merlinofchaos’s picture

3) The auto-alphabetizing of topics seems odd. There is rarely a case
that alpha is the correct order for pages. I'd simply assume that the
order they are in my .help file was the order they would be displayed.
Other than being the expected behavior (from my perspective, I know) it
would also do away with weights, which all around is a good thing.
Weights are the devil's own and tend to cause more confusion than help.

I almost agreed with this and then I remembered that weights are critical for allowing modules to insert items into other module's trees. Without weights it cannot easily be done.

4) Why do we need the funky syntax stuff in the html for links/images and is there a reason we need that *specific* syntax ie. the & versus some other token-like symbol)? I'm sure there is a good reason, but I haven't researched at all. It frankly looks kinda weird and confusing for some reason and I wonder, depending on the whys if we can use something that visual looks cleaner. Then again, maybe that's just me.
For those just reading along , this is what I am referring to:
<a href="&topic:module/topic&">
<a href="&base_url&admin/settings/site-configuration">

The & was chosen so as not to interfere with other tokens but there isn't any specific reason we couldn't use another token. It replaced an earlier system I used that was slightly less flexible, but is necessary given that it's impossible for the help files to know your site's URL structure. This can be changed around I am sure.

redndahead’s picture

We chose & symbols because there is an easy html entity for & when you want to create a documentation page for creating help files. That said, it was pointed out that there is a numbered entity that we can use to create [] so this could possibly change.

One item that could change just for consistency is use &base_url:admin/settings/site-configuration& but that's not as token looking.

add1sun’s picture

Re: the weighting, gotcha. I still think that the default order to display pages should be the order they are in the .help file, rather than alphabetical though.

Re: the token symbols, yeah I guess I find them visually confusing because I don't see & as delimiters and expect that to be a connector instead. (I'm not sure that makes sense to people not in my brain. ;-)) Using some other symbol like brackets [ ] or braces {} would visually work better for me. Not sure which symbols conflict for us.

I have a long flight tomorrow so I plan to spend more time on this doing code review and fixing up the example files so they are accurate. I am also going to work on setting up a demo site this weekend that has the patch applied so I can get more doc people playing with this.

Xano’s picture

I'm with add1sun on the tokens. Using brackets would make them more distinguished and it makes them easier recognisable as tokens, since Token uses brackets as well.

What about my suggestion for a PHP callback token in #131?

add1sun’s picture

FileSize
224 bytes
8.59 KB
40.74 KB
Failed: 9273 passes, 62 fails, 22 exceptions View

The patch attached is keeping up a bit and taking care of offsets/fuzz. The new zip file has minor fixes to the example help files and removes extraneous Mac files. The Help module help files still need a real going over but at least it should be a bit more accurate. I also attached the same .png just for completeness. As above, apply the patch, unzip the examples from your Drupal root (so the help folders end up in the module dirs) and put the .png in modules/help.

I still need to find some time to get plain old code review done. Hopefully I can wrangle some folks at SANDcamp this weekend to give it a good poke as well. Sun was quite awesome and has upgraded Demo module to the latest D7 unstable so I will also be setting up a demo site this weekend so we can get more "non-coder-types" to look at this.

Further notes and questions:
The bullets look funky on regular help pages due to Garland, line 106, setting a tiny margin margin:0.15em 0 0.15em 0.5em;. They are fine in other themes that don't monkey with that. I'm not sure why Garland is adding it. I didn't check out all the locations of lists with bullets but removing that tiny margin and letting it fall back system.css default looks fine to me.

On a related note, on the regular help pages, children are not indented under their parent because its <ul> is in there as a <div class="item-list"> rather than an <li> which would indent for us.

One thing that kind of throws me off is the default about.html. I sorta feel like we shouldn't do that. Having a file that appears but is sometimes listed explicitly in the .help and other times not seems like it will be more confusing than anything. If someone wants an about page, they should just add it to their .help like anything else. Is there a special case or reason we need to have this auto default?

I tried removing the part of the sort function that did an alpha sort, with the hopes it would be that simple. ;-) Doing that means that the weights work, but the pages list in reverse order. That's not so hard to tangle with, but moving the order of the page with a weight, effects other unweighted page orders. That is, a -10 weighted page will always print in that weighted order but if it is listed at the top, versus say the middle, of the other pages, the other pages will reorder in odd ways. So I'm guessing we need to find the order in the file and explicitly set it or something. Too tired to monkey with it more right now.

Status: Needs review » Needs work

The last submitted patch failed testing.

keith.smith’s picture

I've updated the demo site at http://helpdemo.advantixllc.com with add1sun's patch in #137, made new foomodule.help files and combined those with the about.html files I had previously created for each core module (#101). The demo site should now show the current code with help files for all core modules.

I note that "Content translation" module (translation.module) shows up in the "T"s rather than the "C"s, so that's at least one action item.

merlinofchaos’s picture

#140, aggregated action item list for this patch:

  1. Remove or retool the automatic 'about' functionality. It's had few positive comments. I prefer remove and replace with something like I outlined in #127 -- this could also be done as a separate issue.
  2. Move the gradient to Garland; fix garland's css to not trample these lists.
  3. Resolve the & symbol in the tokens. I personally also prefer [] and if we can't find a reason not to use this, let's use that.
  4. #131: callback function to insert dynamic items into these pages. I'm still not comfortable with that as it harms the utility of these as truly standalone documents, but it could merit further discussion.
  5. If we can resolve weighting with file order instead of alpha sort, great. I'm all for leaving that as is and immediately opening a new issue to fix/change this after this patch is committed.
  6. Update the help.module help to be accurate and current.

Did I miss anything?

keith.smith’s picture

A note on #140-1: I had to create modulename.help files in order for the about.html files to be recognized, so at least part of the "automatic" process found in earlier patches has been removed in #137. But, add1sun's notes in #137 suggest that this isn't the case, so perhaps something else is going on here.

catch’s picture

Issue tags: +i18n sprint

Tagging with i18n sprint.

Jose Reyero’s picture

Issue tags: -i18n sprint

Subscribing.

And one question: could this (or something similar) be used to move other big texts out of the code, like default email texts, or default content type descriptions?

yoroy’s picture

Issue tags: +i18n sprint
add1sun’s picture

Re: #140:

I'm cool with half of those breaking out into their own issues after we get this inital patch in because the system works fine without monkeying with them. So that is, #1, automatic "about" functionality, #4 adding callback functionality, and #5 changing default weighting, all go to new issues.

I agree with Earl on the other 3.

merlinofchaos’s picture

Jose: Technically yes, but it doesn't feel like the right place to do it. Those aren't really help texts, even though I can see the utility of doing that.

merlinofchaos’s picture

Updated from add1sun's comments:

Active action items:

  1. Strip out the automatic 'about' functionality entirely.
  2. Move the gradient to Garland; fix garland's css to not trample these lists.
  3. Resolve the & symbol in the tokens. I personally also prefer [] and if we can't find a reason not to use this, let's use that.
  4. Update the help.module help to be accurate and current.

Once this is in, these action items should immediately get their own issues for discussion, in addition to other stuff Gurpartap already wants to add:

  1. Create an issue to discuss my ideas in #127 as an alternative to the automatic 'about' functionality.
  2. #131: callback function to insert dynamic items into these pages. I'm still not comfortable with that as it harms the utility of these as truly standalone documents, but it could merit further discussion.
  3. If we can resolve weighting with file order instead of alpha sort, great. I'm all for leaving that as is and immediately opening a new issue to fix/change this after this patch is committed.
add1sun’s picture

FileSize
45.52 KB
Failed: 9407 passes, 18 fails, 0 exceptions View

Ugh, I'm a retard. The last patch is bad because I forgot to manually add the new files. Gak! New *complete* patch attached. Use the same example and image files from #137.

add1sun’s picture

I should also add that I have finally gotten a demo site up that wipes every hour. This allows people to have access to the module admin screen to see how the inline popups work as well. I will fill out more info about the new system and keep it up with our work (even after this patch goes in, with the followup issues).

http://help.drupaltest.org/

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
45.14 KB
Failed: 9381 passes, 63 fails, 0 exceptions View
  • I have not changed about.html behavior. I agree with your argument, but we have no better solution to handle "main" page of module help "yet". It is also proposed that #127 shall replace that, in a follow up issue.
  • We don't load any theme in popup, it's all help's styling. Hence, there's no chance we can put this in garland. And we don't use gradient on non-popup page(where garland is used). Also, the gradient.png in garland is inverted.
  • Available tokens:
    • [topic:module/topic] to the URL to the help topic.
    • [url:admin/build/block] to the URL to the site.
    • [path] to the URL to the base help directory.
    • [trans_path] to the URL to the actual help directory.
    • [base_url] to the URL to the site.
  • If anyone interested to update help text, most appreciated. I have rearranged, and renamed stuff there. About file now has "Why ah?" content.
  • Tests will fail because there's no help for modules except color and help itself.
  • Also fixed some interface issues.
  • gradient.png included in tarball.
  • Good night.
tstoeckler’s picture

I'm guessing *complete* translates to CNR... edit: I'm stupid, sorry...

add1sun’s picture

Thanks Gurpartap! I've updated the demo site with the latest patch. Quick responses:

I say we ditch the gradient.png altogether. I find it unnecessary and maybe even distracting (I like things clean, so maybe that's just me).

For the tokens, is there a reason to have url: and base_url (not saying their isn't but this is an uninformed perspective)? I'd also like to name path and trans_path better because those are clear as mud as to what they are pointing to for me. What is the difference between "base help dir" and "actual help dir"?

Shouldn't we add Keith's help files from #101 to the patch so we have them all and tests will pass?

Status: Needs review » Needs work

The last submitted patch failed testing.

merlinofchaos’s picture

Status: Needs work » Needs review

I have not changed about.html behavior. I agree with your argument, but we have no better solution to handle "main" page of module help "yet". It is also proposed that #127 shall replace that, in a follow up issue.

What we have right now is kind of broken though, so we need to remove it. Specify the main page in the .ini file with a keyword; that will do for now. It seems like it should be trivial to do [help settings] main = [pagename]

And that basically handles the 'about' logic just fine.

I'm with Addi, I don't see a need for the gradient in the popup.

Addi: trans_path is used if you are using help out of a translated help. THis is necessary if you are translating images; sometimes they don't need to be translated so you want to use the original, sometimes you want to translate the image so you specify, thus trans_path.

I'm not sure offhand what the distinction between url and base_url is.

yoroy’s picture

One more vote to please just drop the gradient. Besides the fact that rotating it totally breaks visual consistency with Garland, it's just not necessary at all.

Jose Reyero’s picture

Status: Needs review » Needs work

I like how this is looking. But I think this misses some important things like on-page translation/localization, and also I'd like to see some more general feature to handle text/html files in Drupal.

The point about localization is that with this new system you cannot use the UI (locale) nor string overrides to create a translation for a missing help page in one language,

This is more or less addressed here, that I know partly overlaps with this patch, but also we could merge some ideas, see #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text()

merlinofchaos’s picture

Status: Needs work » Needs review

While I think that system might be good for embedded text, I think it might fail for help texts, where you may find yourself translating more than just text. These are really, when you think about it, entire works. They were set aside like this so they can be translated completely independently, as an entire work, complete with context.

This system also has an advantage that you can write your base help texts in whatever language you like (it doesn't have to be English) and the translation system can pick up the proper change.

Yes, this stuff is difficult to translate in place, but pushing the translation into the database adds a burden to the system that is unnecessary here; it adds a burden to the help text writer because the benefit of having completely separate HTML files is destroyed and now you have another .po like file to deal with, only now *everyone* has to deal with it instead of just translators.

One of the benefits of this help system is its ability to be standalone, and I don't think marking it CNW in favor of a proposed system that is further away from reality than this one. We are on the brink of this being ready to go in, 5 months and 150 replies in, and as far as I can tell you just fired a giant torpedo at it, because the translation system provided (which we discussed on the dev list a year ago) doesn't extend to the rest of the system. Even though one of the *intents* is that the help system is separate.

Jose Reyero’s picture

@merlinofchaos,
Basically agree, and I really would like to see this help patch in.

However I think there are important advantages in some uniform handling of text in files (most of them are html anyway). They can be overridden by the locale system (which doesn't mean you need to store the full source into the db, though you could), or handled uniformly for translated files. Actually the other patch looks in the module/text folder, then for a translation there, then in a different '/translations' folder which is aiming at language packs (someday).

So I'd just ask a pair things here:
- That all files are stored in a 'text/' subfolder with a 'help-' prefix or similar so later other big texts can be added in
- That we use a generic enough function for grabbing the text files

If we use some good handling for these file includes, we could also translate them through the web (provided a suitable UI is built) and store such translations into a '/files/translations' writable folder (and ready to be exported and contributed into the cvs btw)

add1sun’s picture

FileSize
45.02 KB
Failed: 9366 passes, 62 fails, 0 exceptions View
65.24 KB

Not much time to devote to this today. Here is a new zip of help files. This has one for each core module, as provided by keith.smith earlier. I added a .help since we are not going to rely on auto locating about.html. I put the latest Help module files, from #150, as well.

The zip and the patch also remove the gradient.png.

For testers: apply the patch and unzip the core-help-files.zip in the Drupal root to place the help files in all of the module dirs.

Status: Needs review » Needs work

The last submitted patch failed testing.

Gurpartap Singh’s picture

Status: Needs work » Needs review

Thanks add1sun for the push! :) I'll make sure to submit new patch tonight incorporating the discussed features.

Pasqualle’s picture

minor issue

+function help_by_module() {
...
+    else {
+     $name = t($file->info['name']);
+    }
...

The module name should not be translated

Noyz’s picture

First, thanks so much for focusing on this issue. The help system is very weak. While I think its getting better, I think we could make some improvements

Before commenting, allow me to get on my horse and preach for a moment. A design cannot be perfect for 100% of users. That's said, the goal of any design should be to solve for the 80% case. The remaining 20% can be a sub-optimal experience. Why am I preaching this? Read on...

1. Drupal requires too much reading. I'm glad the language changed from "more help" to "get help", but I think the text all together is unnecessary. Instead, put the text inside of a title tag, and rely solely on the icon. The "?" is universally understood as help, so it doesn't need to be accompanied with text. 20% of the users may want the text, but 80% will understand this without text. Let's do the Everyman a favor and cut down on the amount he/she has to read when using Drupal

2. Pop-ups are OUT. Everyone (probably including you my reader), has lost faith in pop-ups. They've been attacked, hacked, blocked, what have you. The new pop-up is the modal window. Number 6 on smashing magazines web trends of 09 (http://www.smashingmagazine.com/2009/01/14/web-design-trends-for-2009/). IMHO, the single biggest improvement one can make to Drupal is to start taking advantage of the modal window. Modal windows force users to focus in on the task at hand - in this case getting help. Anytime one clicks help, they should get a modal window.

webchick’s picture

@Noyz: While I don't want to start a bike-shed about pop-ups vs. modals vs. tool-tips in this issue (I think this is a good discussion to have, but these kinds of things that can be massaged after the initial patch has landed), I *will* say that I think modal dialogs are exactly the wrong kind of UI mechanism for the kind of help we're talking about here.

This help system is intended to be a replacement for the kind of help you get with desktop applications (usually found under Help > $application Help). It's imperative that you can see both the help window and the interface you are trying to deal with so that you can play around with the settings while you are reading the docs. Otherwise you would need to switch in and out of "modal mode" and your user experience becomes:

Look at the docs. The docs show a screenshot of an interface. Close docs. Move to interface. Open docs again. Read another sentence. Close docs. Perform steps. Open docs. Read another sentence. Furiously close laptop and walk away before your computer turns into a $2,000 pavement adornment.

I do think modal dialogs have their place with tasks that make sense "walled off" from their surroundings. I just don't think help (or at least this style of help) is one of them.

nadavoid’s picture

My eyes are pretty fresh on this issue; I haven't looked at it much. Today I looked at the "creating-help" page for the first time. There is one thing that confused me, and I would like to see clarified.

I see in bold things like this: <a href="[topic:module/topic]"> or <img src="[path]example.jpg" /> or <img src="[path]example.jpg" />. With my fresh newbie eyes, I don't know if I'm supposed to replace [topic:module/topic] or [path] with something else.

I honestly don't know what I should do with what is in the brackets, so using it would be a process of trial and error. I would probably start off using what's in the brackets verbatim.

What I would like to see in these cases is an example of what proper code looks like, and why.

Example of what I wold like to see...

Use <img src="[path]example.jpg" /> to reference items within the help directory, such as images you wish to embed within the help text. For example <img src="example.example.example.jpg" />

or

[path] gets automatically replaced with "foo". Use it in your help.html files, and it will be converted into a proper path, for example /modules/all/foo/fred/help/.

OK, those are obviously made up examples with dummy info, but hopefully that gives a picture of what would clarify things a bit for me.

Noyz’s picture

@webchick: You make a good point. Sold (for now).

If we fast forward to the day when everyone finds Drupal intuitive, the need for a help system that requires such back and forth suggests the UI itself was designed incorrectly. I guess my mind is in the future where little help is needed. My optimistic side I guess.

webchick’s picture

@Noyz: I very much welcome the day when our help system lies completely dormant, never clicked on by any users because Drupal is just so darn easy to use they never need it, and am looking forward to working with you to get it there. :D

merlinofchaos’s picture

If we fast forward to the day when everyone finds Drupal intuitive, the need for a help system that requires such back and forth suggests the UI itself was designed incorrectly.

One thing I've learned about working on Views -- lots of people try to say that complexity lies 100% in the UI, but that is simply not true. Let me use Views as an example:

There is absolutely nothing I can do in the UI to hide the fact that you need to understand the basic relationship between a node, a user and taxonomy terms to be successful. On a micro level, it turns out you might also need to understand the relationship between a node, a user, and a file to be successful (if one of your goals is listing files). At the end of the day, it's not the UI's fault that this understanding is necessary, it is fundamental to the system. The UI cannot replace this understanding, the best it can do is try to smooth it over, to guide and inform. But it cannot read your mind and know what you want, and you cannot do what you want out of ignorance. Therefore, the information must be available.

There are a lot of concepts you need to understand to be successful using a system like Drupal and good UI does not mean hiding those concepts. It means being able to explain those concepts when you need them in a way that is not intrusive, yet is there to provide what you need when you need it. It requires a lot of thought, good writing...and a system that can present varying amounts of information without interrupting your current task.

Aren Cambre’s picture

There are a lot of concepts you need to understand to be successful using a system like Drupal and good UI does not mean hiding those concepts.

All that proves is we haven't thought of something more intuitive. It doesn't prove that it does not exist.

Maybe someday users won't need to intimately know relationships? I can't guess whether or how that day will come, but I'm not ruling it out.

yoroy’s picture

Agree with webchick on the usefulness of a pop up in this particular scenario. I'm not able to actually test the latest patch, but I can try and keep things on track, so:

Active action items for landing this initial patch:

1. Strip out the automatic 'about' functionality entirely.
2. fix garland's css to not trample these lists.
3. Resolve the & symbol in the tokens. Use [] and give examples of correct usage (see #165)
4. Update the help.module help to be accurate and current.

Please update and repost this list if the latest patch already fixed some of these, it helps maintain focus.

add1sun’s picture

AFAIK, the last patch resolved #2 and #3 in the code, though better docs on #3 are probably still needed, so that falls under #4. So we have really:

1. Strip out the about functionality
2. Update/clarify the help.module help (the placeholder stuff and stuff in #165 for sure)

Psikik’s picture

I just checked this out on the demosite. Neat and useful. But the one glaring omission I noticed was the lack of a close button near the bottom of the help text (say around the Up link).

merlinofchaos’s picture

What's wrong with the browser's close button? I've not quite understood why that should be duplicated?

Gurpartap Singh’s picture

There's no reason to put a close link. If I have to imagine one, I'll say because popups on some websites provide it.

rickvug’s picture

@merlinofchaos - I'd find the close link helpful and consistent with other pop-up windows online. In addition, I could see the help system pop-ups working well with thickbox or a similar javascript effect. In this case the close link would be especially helpful as there would not be a browser close link. Either way this is a small detail.

Jose Reyero’s picture

The issue with the close button is not that it is any use, it's just that people are used to it (if you find something works like that 90% of the time, make it a usability guideline)

That said, I wouldn't add it, but don't listen too me too much, I hate pop-ups anyway :-)

Btw,
a) is there any more options for display more than pop-ups here? I mean you can also use a js overlay, load it inline (as a drop-down + ajax), etc...
b) What about print button? Do we have one?
c) Will I be able to build a popup-less theme still having help texts?

merlinofchaos’s picture

The links are all themable, so yes, a theme could always eschew popups entirely if it wants. Not sure how it would address the "data loss with form" problem which is why these popups exist.

I fully intend to extend the D6 advanced help with Jeff Robbin's beautytips when I get a chance. I don't think it will be very difficult at all to do this, it would happen purely at the javascript layer I think, though it might require a special ajax callback to get the text.

Any close button that a javascript popup uses would be fundamentally different in operation than what a non-javascript popup uses, so supplying one wouldn't help. Also, for it to function, it would, itself, have to be javascript.

merlinofchaos’s picture

b) no print button at this time. Should be either doable or work just fine with print module.

Jose Reyero’s picture

Really, I feel we are losing more features every day with this patch. Help texts are not localizable through the UI (critical IMHO), you may not be able to print help text without some copy and paste (js popups in my browser are not easy to print)...

I wish this could be split into two different issues, which I think are completely unrelated. We shouldn't be mixing both here

1. help text in files
2. help text as pop-ups

merlinofchaos’s picture

Jose: The entire text is always available not in the popups. The popups are only for specific links that are used to try and avoid people navigating away from a form with unsaved data, which is deadly UI.

I'm not sure I understand why localizable via UI is a stopper here. Maybe we can add something on to make this possible via a patch but I simply cannot imagine translating this text in a browser window: http://views-help.doc.logrus.com/help/views/new

Especially since this is admin facing, not user facing, and is the kind of text that is shipped with the module and never tinkered with.

merlinofchaos’s picture

And there is NO JS POPUP so that's not a problem with print. And i don't see a 'print' button today, so how are we losing a feature there?

Aren Cambre’s picture

Another vote for no close button. What else does the big fat X mean on the top right side of the popup?

I disagree that it's standard for popups to have close icons. Maybe MODAL windows, but not popups.

Jose Reyero’s picture

@merlinofchaos
Forget abt the print thing, my mistake, I understood we were using js popups

However, about the localization thing, yes, it is a big problem (if you don't live in an English speaking country): With this new system, to install/update a translation means installing files into the filesystem (Upload files), while before this all you had to do was to enable language / import po (100% web UI)

While on one side we are working towards making Drupal usable/ installable in other languages (The only English page used to be the first one, on which you had to select the language), this is a step backwards. I'd call this a show stopper.

tstoeckler’s picture

With the current localisation system you also have to upload the translation files in to your drupal root directory upon install. Unless someone, of coursed has already packaged a localized installation, which would work with the help files as well, though. So this system is not in any way a step back in terms of localization.

Jose Reyero’s picture

@tstoeckler
Not so, you can upload po files using the UI => all Drupal translated.

Anyway, does this patch even allow translation of module help pages, by any method?

gaele’s picture

With this new system, to install/update a translation means installing files into the filesystem (Upload files), while before this all you had to do was to enable language / import po (100% web UI)

Being from a non-English speaking country I believe José has a point here.

Pasqualle’s picture

I think every translation should be committed to CVS. The only good source of translation is when it is packaged with the module.

Current translation processes:
a)
1. extract strings into .po file
2. use a po editor to translate the strings
3. upload the translation to your site

b)
1. use the l10n_client to modify some translations online

c)
1. use the l10n_server to translate the po files
2. use a po editor or translate the strings on the server
3. upload the translation to your site

In every case a, b, c the last step would be to upload the finished translations to d.o, because most users do not know anything about the translation process.. And the ideal situation would be when they do not need to care about the process..

We do not have an option to change translation files online. So with help files you have to.
1. download the help files and pictures
2. translate the help files with your favorite html editor, translate the pictures with your favorite picture editor
3. upload it to your site
4. upload it to d.o

Check the help folder in the views module. That help is not complete, but it is very useful. Do you really want to translate all those files online?

webchick’s picture

It seems to me there are two outstanding complaints with regard to this help system and translations:

1. We lose the ability for people to translate this help text online via Locale module and similar tools. The massive amount of text and images we're talking about here in what we hope to become a typical help file leave me skeptical that people actually would do this in a web browser and not in an "offline" tool like Dreamweaver. I'm not sure that this is a show-stopper, per se.

2. We lose the ability for people to upload already-translated help through a web interface. People now need to learn up to three different ways of translating things to their site: a) uploading a .po translation file for interface strings (web), b) creating a 'translation' of a piece of content such as a node, menu, or taxonomy term (web), c) uploading a help translation (ftp).

I can think of a couple ways to resolve this. One is blessing a "magic" directory in the files directory (files/help/translations, perhaps) where, if help files were found, would be treated as translations. (e.g. files/help/translations/hu/index.html, files/help/translations/es/index.html). That would allow people to upload "help packs" through a web UI without requiring write access to other places in their file system. Another is solving the bigger problem of "how to translate big chunks of text + images through a web interface," which would be quite another matter.

It seems to me that given the size and scope of this patch already, that it already dramatically improves on the existing help system in just about every possible way, and it has the docs team "thumbs up" as enabling them to do their jobs much easier, that we could look at resolving this translation problem in a follow-up patch after this patch lands. I'm concerned about how long this issue has been drawn out (we first learned of the "disappearing help" issue nearly a year ago), and it's something I'd really like in core in preparation of the usability testing later this month at University of Baltimore.

However, I realize that I also am a native English speaker with mostly English-speaking clients, so I would like to hear from José, gaele, and others as to whether there's any compromise here.

merlinofchaos’s picture

Jose, I'm okay with a *_get_text() alternative to finding translations as long as we don't change the primary storage format. My biggest objection here was that it appeared to me, at first blush, that you suddenly have to put all your texts in one file, and that would become a very serious burden on documentation writers with the help system. If, however, we treat that as an alternative in the help system, then it should be no trouble and can be patched in.

I do still think we should consider it an alternative translation format, because I strongly believe that documentation translation is the kind of job that needs to be done in the best tools possible -- your HTML editor of choice -- and not a browser, and should be packaged with a module (or even separately so that it can be maintained independently).

Does that alleviate your concerns about translatability? I would say we could address that after this patch goes in with your large text translation concept and figure out a way to smooth things out.

Jose Reyero’s picture

I was thinking I'd stop whining and start coding to address that issues... but really this is far from being ready yet...

I find it is not working for me at all, urls not rewritten properly, no pop-ups anywhere (or when I force the pop-up &popup=1, the links to admin pages open in the pop-ups, etc, etc... [how are we going to fix this last one?]

(I even reinstalled the whole thing with the files in place, no luck. Btw, the zip package is plenty of dirty files...)

Besides, I find the code is not up to standards, quite messy and plenty of dirty string concatenation: "$module_path/translations/help/$language->language/$module.help", $file = "./$file_info[path]/$file_info[file]".......

@webchick
About 1. ok, as long as this is used only for module help. However it would be a big issue if used for other help texts, in forms, etc..

About 2. That is a real issue, when you think of end users updating their translations. I have also the concern that the amount of contributed translations will go under with this.
If you compare 'get the web browser, translate this and this, share the po' with 'get the file, open it edit/save, upload to the server, check, send the file..'. And we had the l10n client and server which already allows remote posting translations automatically to a central server.... :-(

Really, is it that difficult to allow overriding the translations with the localization system? (two overriding levels: variables, localization, files in multiple places, just a start but.. #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text()

> given the size and scope of this patch already....
Yes, too big, one of the reaons is that it addresses two issues in one (pop ups, help in text files). Given how the code looks yet I'd still split this thread in two.

@merlinofchaos
> Does that alleviate your concerns about translatability?
Not really
> I would say we could address that after this patch goes in..
... :-)

Still, more real concerns about translation. A translator is not a web designer. Translators are supposed to deal with texts, not with html or Dreamweaver. Worse yet, these texts are not web pages, are chunks of HTML, so I can't wait to see the translations we get using Dreamweaver.
I am sure this patch will really boost the quality and quantity of the English documentation delivered with Drupal, but also I think the number of full translations of Drupal will go close to zero.
I really don't understand how this is better than keeping a good online handbook, that you can update always in real time. Also I think we are mixing here the concepts of 'help text' (always quickly available, on page, up to the task) with 'documentation' (pages and pages, like for printing and reading...)

And last point, to finish, how is this better than having a /html folder on which you can add tons of documentation, creating full web pages properly linked?

And with that I promise I won't complain anymore, have a nice day -- in the English speaking world ;-)

merlinofchaos’s picture

Besides, I find the code is not up to standards, quite messy and plenty of dirty string concatenation: "$module_path/translations/help/$language->language/$module.help", $file = "./$file_info[path]/$file_info[file]".......

Um. That's perfectly acceptable by our coding standards.

I find it is not working for me at all, urls not rewritten properly, no pop-ups anywhere

Did you try the demo site? The popups only appear in select places (right now I think the modules page) because they're not meant to be everywhere. Not sure what you mean by urls not rewritten properly.

(or when I force the pop-up &popup=1, the links to admin pages open in the pop-ups, etc, etc... [how are we going to fix this last one?]

Don't force the popup. There's no way for the server to know what kind of browser the content is really in; ?popup=1 is the signal. If you force it incorrectly, then it doesn't work. I'm not sure what the problem here is.

@merlinofchaos
> Does that alleviate your concerns about translatability?
Not really
> I would say we could address that after this patch goes in..
... :-)

Well then I guess we're screwed then. I offer up pretty much what you want, as far as I can tell, and you say no. So what the heck am I supposed to do? Yes, you're right, translations are more important than documentation. Let's live with our current crappy documentation so that it's easier to translate and shoot down this patch.

Clearly I've been documenting Views all wrong. The answer is that I should just stop doing it so that translators won't be fussed with it. You don't have to translate the online handbooks, after all. Even though they're a nearly unmaintainable mess with no version control and laissez faire. But at least you don't feel compelled to translate them because they're not shipped with the module!

Jose Reyero’s picture

Put it in a different way: For some people living in other places untranslated documentation is useless. So I rather have a little translated than a lot untranslated shipped with Drupal.

About 'after this patch goes in', I just meant you never know when the next one will go in (provided someone has the time to code it)....

What I find is that it's quite discouraging that while we are working on making Drupal more translatable and multilingual-friendly (i18n sprint) we move in the opposite direction.

Anyway, I say if the documentation team wants it they should have it.

However we should ask ourselves what are we doing wrong when we are building this great CMS and people working on it prefer to keep content (documentation = content) in the cvs (and editable with Dreamweaver) better than on-line. I really had thought some day my help texts would update automatically from a central repository rather than uploading files.

merlinofchaos’s picture

However we should ask ourselves what are we doing wrong when we are building this great CMS and people working on it prefer to keep content (documentation = content) in the cvs (and editable with Dreamweaver) better than on-line. I really had thought some day my help texts would update automatically from a central repository rather than uploading files.

Actually putting the documentation in CVS facilitates having them online. It gives us version control and it gives us documentation that we can safely use on *.drupal.org. It is a reflection of the success of http://api.drupal.org -- only this is an intermediate between the documentation being purely with the code and the documentation being next to the code.

That's why http://views-help.doc.logrus.com is so popular: it's the documentation that ships with the module and it also is available online. It's the same and it's manageable. It is fully translatable, and I realize that the translation capabilities are not quite what you want right now, but to be perfectly honest, I don't understand exactly what it is you want anymore, since you poo-pood what I thought you wanted.

And the really important thing: When the code changes, the documentation can change with it. If the documentation is completely separate (i.e, in *.drupal.org):

1) You can't translate it at all (this, you can translate)
2) There's no easy version control or version matching (I can keep my documentation in sync more readily when I have different versions of my software)
3) There's no direct link from the module to the documentation (the admin/build/modules page there's a link right there)
4) The documentation is constrained to the resources of editing handbook pages on *.drupal.org -- and I gotta say, I *hate* editing documentation in a browser. I've written a lot more documentation for Views 2 with this system than I ever would have with the old help system where I had to put the documentation in code. And the problems with the documentation for Views on drupal.org remaining in sync with the code? Terrible. It's a constant problem.

Now, I say we take what we have, and we find a way to make the translation even easier and more powerful, and go with that. I still say that the only stumbling block we have is that right now it only looks for translations in the translations folder. But once we get a larger text translation system in place, this should be able to easily integrate into that.

Pasqualle’s picture

I think the number of full translations of Drupal will go close to zero

I fully agree, but I do not see that as a bad thing. If translators are aware that there is an untranslated documentation within Drupal then there will maybe translate it someday. They won't translate anything outside Drupal and contrib modules..

For some people living in other places untranslated documentation is useless. So I rather have a little translated than a lot untranslated shipped with Drupal.

Untranslated documentation is still better than no documentation. There is no translated documentation about advanced topics, and d.o handbook pages will not be translated.. If we can categorize the help files by level (like basic, advanced, developer) then it will be still possible to have that little translated as now and call it as a fully translated Drupal with basic level help..

I believe Drupal needs these detailed and structured help files shipped with the application. There are many feature requests (auto update, categorization, access control) for the new help system, but I think all can be solved. The translation will be harder as these are not strings any more, these are help files..

sun’s picture

We had another IRC discussion on this today. Starting with the topics: I'm against holding off this patch, because

- Site builders/owners won't translate module documentation to their language(s). Only contributors (to Drupal) will. So, what we need is rather a static translation system here. For example, it once took me ~2 days of effective work to translate Panels properly into German. That was volunteer work for the sake of completeness. I was able to commit the changes for the next release right afterwards. If anyone takes up this challenge for any Drupal module, they will not do it for single client, project, or site only, because that would be nothing else but insane.

- This means, if the translation for Swedish exists, take it. If not, use the site's default language. If that does not exist, use the module's default help.

- If the translation of a module improves without my influence, I should get the improved translation. This is a major difference from all other translation systems in Drupal core. All others assume that, even if a module ships with a new translation file, you keep your existing translations. So, effectively, in case of the new help system, we want the opposite from happening. (though that could be advanced in later on)

- The difference to the current help and translation system is that translators used to be able to translate help content by using Drupal core's interface translation UI. There is no such UI for those new help texts yet. However, users could update translations of their local site by adding or altering help translation files in the filesystems. (note: also referring to first point here again)

- So, in an ideal world, the translation process would be to store "custom" translations locally (in DB) and parallely submit them to a translation server. When new translations are available, the site owner/builder can decide whether he wants to keep custom translations or upgrade/migrate to new ones. This should be our ultimate goal regarding module help-text translations.

In general, though, this has nothing to do with the new help system at all. This is a major improvement with regard to usability. To keep it real, only the help text of a *minority* of modules was actually translated in the past. Most often it was left out since translators felt too lazy for it, and concentrated on the real user-facing strings instead. This issue will change the entire value of module-provided strings entirely. So to introduce this help system is already a major improvement on its own. As with internationalization in D6, translation of such strings can come later. Ideally, before D7 is released, of course.

To summarize: A separate issue should deal with the question of

- A translation process to store "custom" translations locally (in DB) and parallely submit them to a translation server.

Already the first part of this statement could end up easily with more than 200 follow-ups.

Let's not hold up this patch with scope-creep.

catch’s picture

One more thing to add, in Drupal , we removed an entire page of help text which hadn't been updated for years, see #189388: Remove miserably broken glossary of Drupal terms from help page. Despite being one of the people who spent a lot of time rewriting some other help texts in Drupal 6, I'm quite happy saying that pretty much all of it is no use - both due to content in some places, and presentation just about everywhere.

We desperately need a system which allows people to modify these help texts outside of PHP - not just for adding pages and pages of new documentation, but for making what's there more concise and accurate. If people are translating completely wrong and misleading help text (and I'm sure we've still got some in core), then that's no use to anyone. It also seems like it'd be pretty easy to integrate it with Jose's approach to long string translation when that's on it's way in.

Gupartap - any update on a re-roll?

Jose Reyero’s picture

Status: Needs review » Needs work

Ok, I see the feature is badly needed (I like it too), though I don't like at all this implementation (read my comments above)

So I've been working on an alternate patch. I'd have preferred to merge it here but I find this code messy and unworkable (Explained in #190)

The option is this one: #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text()

Then we use a simple handler for *all* module help and other long texts (overridable, localizable, etc...), single entry point if we want to add any other feature, can use complex arguments for replacement, etc, etc...

/**
 * Implementation of hook_help().
 */
function user_help($path, $arg) {
...
    case 'admin/help#user':
      return module_get_text('user', 'help_about');
....

and we focus this patch on pop ups only.

I really won't insist much more, I've already complained enough about why this one on its current form is not good so I'll move on and work on other issues.

Pasqualle’s picture

If we really want these help files in the db, then why not save them as nodes? Nodes are fully translatable, you can use WYSIWYG, you can attach images, assign input filters, etc..
But most of all I don't really want to store the help files in my db (I do not see the benefit of it), so I would prefer such functionality as a (contrib) module..

Michelle’s picture

Ack, no. No help files in nodes. Nodes are site content, not admin help. I don't want it coming up in searches and whatnot.

Michelle

Pasqualle’s picture

Help could be a site content, it is not an admin functionality only. For example if you have a module for image upload and you create a nice user help for it, then you want to display that help for users. If you create a field which stores for example a car insurance category, then you wants to explain the categories to users, so you create a nice help file and put a little (?) mark next to that field..
And of course you want to be able to search (in or) for that help.

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
65.24 KB
43.65 KB
Failed: 9668 passes, 70 fails, 0 exceptions View

um yum yum

gaele’s picture

Okay, let's take a step back.

There are several levels of help texts in Drupal:

  1. field descriptions: small chunks of text explaining an input field
  2. page descriptions: large pieces of text explaining what to do on a page (like the introduction on the blocks page)
  3. module help: a full page explaining what to do with a module
  4. documentation on drupal.org: more background information, how-to's, etc.

This "Help system patch" replaces 3, but combines elements of 4. It is a new level of help text, currently not available in Drupal: several pages with background information, structured and organized by topic.

Currently the default US English texts for 1, 2 and 3 are part of the module code. The default US English texts for the new help system will be separate files containing HTML.

Advantages:

  • usable as stand-alone documentation
  • packaged with a module, so they can be put under version control
  • organized by topic
  • configurable as pop-up, so usable in combination with forms
  • allows people without coding knowledge to contribute help texts

merlinofchaos expects that available module help will be widely expanded, as it is easier for people without coding knowledge to contribute.

José says: with this patch we'll add a separate system for translation and import tasks; this system will replace several help texts which are currently managed by Drupal's localization UI. Translating and importing localized help texts will be more difficult.

I think they are both right.

I wonder where we're heading, though:

  • If I understand it correctly the new Help system will also replace 2 (page descriptions) in a follow-up patch (see #7, hook_help will be completely gone). Will these be pop-ups too? How will this fit in the help tree structure?
  • If I give a content editor access to e.g. admin/content/comment, which currently includes a page description, will he/she be exposed to the whole help tree?
  • Will translated help pages be usable stand-alone?

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

Just a reminder that the Formal usability testing of Drupal 7 at the University of Baltimore will be on February 25th-27th.

So if someone could clearly state what else should be fixed in the patch that would be a huge help, as everybody agree that this the feature is badly needed..

add1sun’s picture

Status: Needs work » Needs review

Now that the last patch removed the auto-about page thingy, the thing we really need to get this thing RTBC is a thorough, nit-picky code review and make sure that the test failures are due to the "missing" help files that can't be rolled with it and not that it is failing tests with real issues.

After the patch goes in, here are things we still want to look at:
1. Create an issue to discuss the ideas in #127 as an alternative to the automatic 'about' functionality (which has now been removed).
2. Create an issue to discuss #131 callback function to insert dynamic items into these pages. This needs a lot more discussion to see if this is a feature we want/need.
3. Create an issue to try to have topics weighted by order in .help rather than in alpha order.
4. Look at the large help text translation issue in context of this system. #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text()
5. Continue to improve the help documentation contained in the new system.

add1sun’s picture

FileSize
41.61 KB
Failed: 9727 passes, 78 fails, 26 exceptions View

OK, so I did a little manual testing and it turns out that blog, dblog and forum did not have the 'access help' perm for their help tests. Apparently these were the only modules running their own tests for help. I talked with webchick on IRC and she prefers that help.module do all of its own testing, rather than having other modules look for it, so I am attaching a patch that removes those tests.

The help.test is failing because it still asserts that the title of the help pages will be "About . $modulename", which they are not now. I am not familiar with testing and didn't have time to actually try to fix the tests sufficiently. Talking with webchick and catch on IRC got this:

[17:25] We change the test slightly.
[17:25] To basically do:
[17:25] "Foreach enabled module"
[17:25] "Check to see if it has help files defined"
[17:25] "If it does, make sure you see them at admin/help"
[17:26] add1sun, Looks like there is a help_exists() module that could be used.
[17:29] The current test is *very* thorough
[17:29] But it doesn't really have to be.
[17:29] add1sun: so if help.module has about page, back/forward links, and a link to the help, that's great.

add1sun’s picture

Status: Needs review » Needs work

Annnnd CNW.

chx’s picture

Status: Needs work » Needs review
FileSize
123.07 KB
Failed: Failed to apply patch. View

Sry, thrown out the existing help.test and wrote another. Also the patch contains ALL files.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
121.24 KB
Failed: Failed to apply patch. View

Grrr, grrrr, grr! Some whitespaces in the patch.

add1sun’s picture

Status: Needs review » Needs work

Holy awesomeness chx! Thanks. So, now we need a rigorous code review on this puppy and it should be able to go to RTBC.

add1sun’s picture

Status: Needs work » Needs review

oh ffs, how did i set that to CNW? Changing it back where it belongs.

catch’s picture

FileSize
121.12 KB
Failed: Failed to apply patch. View

Nitpicks. Anything I could fix inline in the patch file I did myself, but no idea how chx got this whole thing rolled.

Stuff I already changed in the patch I've made a note with the comment.

+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
+  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

No opening PHP tag or $ID$

Having to reverse the breadcrumb like this looked a bit odd, but minor and shouldn't hold things up
drupal_set_breadcrumb(array_reverse($breadcrumb));

+ * The name of the module (without the .module extension).
We never specify leaving the extension out usually, so just "the module name" would do. (fixed in patch)

+/* $Id: help-popup.css,v 1.4 2008/04/28 20:40:59 merlinofchaos Exp $ */ (fixed in patch)

+    $output = t('No such help topic.');

Maybe "No help topic on this subject is available."? (fixed in patch)

+  if (array_key_exists('popup', $_GET)) {

We normally use if (isset($_GET['popup'])) - (changed in patch)

+    // @todo Is this trusted output?

Is this still a TODO? Capitalised it in the patch anyway.

+    // Run the line break filter if requested

missing period. Fixed in patch.

Eventually it might make sense to have help_view_topic() provide $output to drupal_render(). Also that function is quite long and could maybe use a couple of helper functions? followup patch though if at all.

+          // If it doesn't exist, top level.

If what doesn't exist? This appears twice in two adjacent hunks of code, could do with expanding. I changed it to "if no parent available, top level" for now.

+/**
+ * Return information for the help topic file.
+ *
+ * This function checks a list of possible locations for a help topic file,

Should just say "Checks a list" since we know it's a function. (fixed in patch)

+/**
+ * Helper function to get a module's proper name.
+ */
+function help_get_module_name($module) {
+  $settings = help_get_settings();
+  if (isset($settings[$module]['name'])) {
+    $name = $settings[$module]['name'];
+  }
+  else {
+    $info = db_fetch_object(db_query("SELECT * FROM {system} WHERE name = :name", array(':name' => $module)));
+    $info = unserialize($info->info);
+    $name = t($info['name']);
+  }
+  return $name;
+}

Do we not have this already in core? If not it should maybe move to system.module at some point.
Also this query should be db_query()->fetchObject(); db_fetch_object() is deprecated. (fixed in patch)

modules/help/help/help.help

Very cool.

While this patch adds all the .help files for core modules, it leaves the hook_help() for those modules intact - so we have the same texts in two places now. Does this just mean a followup patch to remove those lines of code? Or something else?

There's only a couple of minor code style issues left, that I noticed anyway, so leaving this at CNR.

JohnAlbin’s picture

@catch He followed these instructions: http://drupal.org/patch/create#new :-) Which have been newly updated to prevent exploding testbots! ;-)

catch’s picture

@JohnAlbin, yeah that's good for adding a new file, and the directory trick is nice (although I've not needed that yet), but wouldn't fancy doing it on 30+ directories and 60+ files at once. I think chx used bzr magic for this one.

merlinofchaos’s picture

+ if (array_key_exists('popup', $_GET)) {

We normally use if (isset($_GET['popup'])) - (changed in patch)

When you see this logic, always ask yourself, "Does NULL have a special meaning." isset() will be FALSE if it is actually there but NULL, while array_key_exists() will be true.

So that means that http://www.example.org/help/module/topic?popup works with one and not the other.

I don't think it actually matters if that doesn't work, but should be thought through.

starbow’s picture

subscribe

starbow’s picture

My initial user testing impressions:
* I had to up the fuzz factor from 2 to 8 to get this patch to apply. No idea if this is significant.
* Starting on the /admin/build/menu page, I click the "More help" link. The cursor immediately turns to a combination pointer/hourglass for two seconds. Then it turns back to a normal counter and nothing happens. I click the link again. Still nothing. I think it must not be working and click to go to a different page. Then the (two) popup up windows open up. Closing them and trying again, I count 10 seconds between when I click the link and the new window actually opens. This is firefox 3, on windows XP. On IE7 on the same system the new window shows up in about a second. On Chrome it pops up instantly. Maybe it is a problem with my Firefox extensions.
* The new window is url http://localhost/head-test/admin/help/menu?popup. It is pretty much empty, with just a "Help" link, the "Menu" header, and an "About Menu" link.
* I click "About Menu". This looks good. Why didn't the "More help" link on the original page take me here?
* I click "Menu module". It takes me to the Drupal Menu handbook page. And now I am stuck because there is no back button. Fortunately I know the keyboard short cut to go back.
* I click the root "Help" link. Now this is pretty sexy. Very nice help overview page.
* I click "About DBLog". Nice page with useful info.
* I click "up". Not where I expected to be. And there is no "up" link on this page. A little confusing.
* I click "Help", and then "Creating Help". Very cool. I really like the quicklink box for jumping to siblings. I am still not sure what good the empty root page does.

Overall this seems like a hugs improvement over improvement over the d6 help (and a solid step better than my help-in-modal-popup approach over at #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations). It would be good to get the navigation tree a little more polished - I would recommend removing the current subject root landing pages, so you jump from drupal page to the subject/about. Then have the "up" link go to the main help page.

Not sure what is up with Firefox. Once the new window is open, click other help links populates it instantly.

add1sun’s picture

Real quickly, re: the "root" page, we just removed the "auto" about page feature because it was odd to have a file named "about" automatically be the root. Not every set of help docs will necessarily have an anout.html file so we want to make it more flexible with determining the landing page. We haven't replaced it yet, but as soon as this patch lands a new issue will be opened to come up with a better way to handle to the root (as seen in the list at #205). Earl's early thoughts on it is at #127.

Firefox 3 for me opens instantly so I'm not sure about your FF experience. :-/

dmitrig01’s picture

* I had to up the fuzz factor from 2 to 8 to get this patch to apply. No idea if this is significant. <-- significant in that it no longer applies :D

add1sun’s picture

So, from catch's comment above we have:

Patch fixes still needed:
1. CVS $Id missing for modules/help/help-popup.tpl.php
2. // If no parent available, top level. comment in help.admin.inc (help_get_topic_hierarchy()) needs to be clarified/expanded.

Questions:
1. Having to reverse the breadcrumb like this looked a bit odd, but minor and shouldn't hold things up
drupal_set_breadcrumb(array_reverse($breadcrumb));
2. Is this still todo in modules/help/help.admin.inc? // @TODO Is this trusted output?
3. Do we not have help_get_module_name() already in core? If not it should maybe move to system.module at some point.
4. While this patch adds all the .help files for core modules, it leaves the hook_help() for those modules intact - so we have the same texts in two places now. Does this just mean a followup patch to remove those lines of code? Or something else?

Future work?:
1. Eventually it might make sense to have help_view_topic() provide $output to drupal_render(). Also that function is quite long and could maybe use a couple of helper functions? followup patch though if at all.

keith.smith’s picture

4. While this patch adds all the .help files for core modules, it leaves the hook_help() for those modules intact - so we have the same texts in two places now. Does this just mean a followup patch to remove those lines of code? Or something else?

I mentioned that in #83, and will volunteer to roll a patch to eliminate the duplication after this goes in.

merlinofchaos’s picture

TODO 2) If I remember right, Clarify: "If this item's parent is unavailable, treat it as top level instead."
Questions 1): taxonomy does this too:

taxonomy/taxonomy.pages.inc:          $breadcrumb = array_reverse($breadcrumb);

It's just easier to build it up in reverse and then turn it around.
Questions 3) The function there is specific to help.module because it gets the module name AND it checks to see if the .help file has overridden it. (CCK requested this feature so that CCK didn't have to have all its help under 'Content'.

add1sun’s picture

Status: Needs review » Needs work
FileSize
47.15 KB
Failed: 9581 passes, 3 fails, 2 exceptions View

OK, coolio, thanks for some of those answers. I have attached a new patch with the two corrections and rolled against fresh HEAD. It is *not* a super patch that includes all of the help files - I don't have time for all of that right now. So if you use this patch you will need the zip file from #201.

Tests run fine, but I get a JS message when visiting the modules page and the popups don't work:

Please wrap your JavaScript code in (function($) { ... })(jQuery); to be compatible. See http://docs.jquery.com/Using_jQuery_with_Other_Libraries.

Remaining question:
- Is this still todo in modules/help/help.admin.inc? // @TODO Is this trusted output?

Future tasks:
1. Subsequent patch to remove duplicate help text from hook_help().
2. Eventually it might make sense to have help_view_topic() provide $output to drupal_render(). Also that function is quite long and could maybe use a couple of helper functions? followup patch though if at all.

add1sun’s picture

Status: Needs work » Needs review
FileSize
47.96 KB
Failed: 9581 passes, 3 fails, 2 exceptions View

New patch with jquery stuff added and a few concatenation fixes.

add1sun’s picture

FileSize
48 KB
Failed: 9581 passes, 3 fails, 2 exceptions View

And yet one more with some indent/white space cleanup. Again, use this patch along with the zip in #201 to get all of the core module help files.

Status: Needs review » Needs work

The last submitted patch failed testing.

add1sun’s picture

Status: Needs work » Needs review

Tests pass locally. If someone can just look at the trusted output question, then this is RTBC:

Is this still todo in modules/help/help.admin.inc? // @TODO Is this trusted output?

add1sun’s picture

Regarding the trusted stuff, we are grabbing an HTML file from within the module. Isn't code within a module "trusted"? This isn't user input, so I think we can remove the TODO, yah?

catch’s picture

I think we can remove it too.

merlinofchaos’s picture

I had the question in there because translators also can modify that text, but I think they are considered trusted as well. So I agree, we can remove it.

add1sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
38.43 KB
62.83 KB
Failed: 9621 passes, 3 fails, 1 exception View

Attached new patch that also includes the help module help files. Also new zip file which has all core module help files, except help module's. This should be RTBC now.

Gurpartap Singh’s picture

woooooooohoooooo RTBC :D

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

add1sun’s picture

Status: Needs work » Reviewed & tested by the community

Needs to have the other module help files to pass the test bot and that is a crazy amount of work. The tests pass locally.

add1sun’s picture

FileSize
65.39 KB
Failed: Failed to apply patch. View

Ok, call me nuts, but the test bot fail bugs me. :-p The test is also checking for the block module help info so I rerolled the patch to add that too. The patch should now pass automated testing. It will still need the zip file too to add the others before being committed.

This is the TODo list after this lands:
1. Create an issue to discuss the ideas in #127 as an alternative to the automatic 'about' functionality (which has now been removed).
2. Create an issue to discuss #131 callback function to insert dynamic items into these pages. This needs a lot more discussion to see if this is a feature we want/need.
3. Create an issue to try to have topics weighted by order in .help rather than in alpha order.
4. Look at the large help text translation issue in context of this system. #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text()
5. Continue to improve the help documentation contained in the new system.
6. Subsequent patch to remove duplicate help text from hook_help().
7. Eventually it might make sense to have help_view_topic() provide $output to drupal_render(). Also that function is quite long and could maybe use a couple of helper functions? followup patch though if at all.

Gurpartap Singh’s picture

Status: Reviewed & tested by the community » Needs work

Code needs work since html files don't follow the new tokens pattern. I'll have a patch in 5 mins.

Gurpartap Singh’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
123.48 KB
Failed: Failed to apply patch. View

Fixed some last minute minute changes I had in mind:
* Fixed token format.
* Changed << and >> into ‹‹ and ››.
* Changed styling for code and pre to something with better contrast (border and background).
* Added some margin to bottom of popup for accessibility of navigation links.

more into TODO list:
* non ?popup links in help popup should open in the larger/normal browser window.

Where art Thy blessings O Bot?

Dries’s picture

I reviewed it and it looks reasonable. However, I'd like us to have another good look at the help system API. The following, for example, was confusing me:

+ /**
+  * Load and render a help topics listing.
+  */
+function help_view_module($module, $popup = FALSE) {

The description doesn't mention module, and the function name doesn't mention topic.

I also don't like the fact that .help files are .ini files, but that .info files are something else. I'd like to see us standardize on a format so there are no 2 'languages' to learn.

Pasqualle’s picture

As I know if you put an .info file into any module's sub-directory, then Drupal (and the packaging script on d.o) will treat it as a sub-module. (Same goes for themes).
So at least the .info file should have a project type property or something, like: type = module (theme or help), or the help directory needs a special case. It is not just that simple to rename these files..
The question would be: do we want to treat the help directory as a project (sub-project)? Because if not, then I think it should not be an .info file as we use that for projects. But Drupal core translations are projects, so theoretically the help could be a project also..

Personally I would not complicate this issue for now..

merlinofchaos’s picture

In the original version, the help .ini files were named .help but Gurpartap and dmitrig01 felt they should be .ini files.

They use the standard .ini parse mechanism because the info file parsing that Steven wrote, which is really great for .info files, doesn't support this format:

[section]
key = value;
key = value;

[section2]
key = value;

And the help system relies on this.

The alternative format, which the .info file does support, is a much uglier syntax:

section[key] = value;
section[key] = value;

section2[key] = value;

You end up with 'section' repeated a lot, which is visually distracting, and makes them more difficult to visually parse.

Gurpartap Singh’s picture

I gave a thought on the .info format for .help files long back during the summer of code. And it turned out to be too much of typing and syntax confusion for novice. Compare here:



// ini format

[about-php]
title = About PHP
file = about-php
weight = -10

[history]
title = History of PHP
file = history
parent = about-php

[usage]
title = Usage of PHP
file = usage
weight = 1

[security] 
title = Security of PHP
file = security
weight = 2

[syntax]
title = PHP syntax
file = syntax
parent = usage

// .info format

help[about-php][title] = About PHP
help[about-php][file] = about-php
help[about-php][weight] = -10

help[history][title] = History of PHP
help[history][file] = history
help[history][parent] = about-php

help[usage][title] = Usage of PHP
help[usage][file] = usage
help[usage][weight] = 1

help[security][title] = Security of PHP
help[security][file] = security
help[security][weight] = 2

help[syntax][title] = PHP syntax
help[syntax][file] = syntax
help[syntax][parent] = usage

From here: http://pastie.org/pastes/224271

kika’s picture

Perhaps way too late for the show -- but what puzzles me is the coupling the UI pattern (popup) and help subsystem / API. Breaking these apart would simplify the patch and concentrate it on API part.

Popup UI element can introduced later, possibly combined with #374646 -- something in the lines of generic dialog UI pattern with single call to invoke a popup box with argument $modal = TRUE, depending on the nature of the popup interaction (do the window beneath needs to be accessible or not).

merlinofchaos’s picture

kika: Apples and oranges. The popup we have is simply another browser window. That is a javascript modal popup. Not even the same thing. It seems like people see the word popup and brain parts just explode. I wish I'd called it something else. :/

Gurpartap Singh’s picture

This shall help for in-browser patch review(although Dries had his look at it already:) http://pastie.org/399938 (excluding help text)

starbow’s picture

@kika & merlinofchaos: I don't think it is worth holding up this issue on, but at a later date it would be good to see if there are enough commonalities between popup-content-in-a-new-browser-window and popup-content-into-the-current-page-with-a-modal-overlay (which is called popbox over in #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations, just to avoid some confusion), that it is worth extracting a common api.
At the very least, we need a common set of visual cues so the user has some clue what is going to happen when he clicks on a link (go to new page, popup window, popup modal, show a hidden fieldset, etc).

Today's fun trivial fact - the term popup is also used in core to describe the autocomplete behavior's suggestions area.

Jose Reyero’s picture

@kika:

but what puzzles me is the coupling the UI pattern (popup) and help subsystem

Yes, I completely agree and feel the same. IMHO this patch is badly scoped from the very beginning, resulting in a huge patch (#190)

Also we lose the ability to display in pop-ups other texts that module help pages. It is badly themeable, and it seems pop up texts can only be in html files (¿are we later creating a text file for each form field we want pop up help for?)

And there's that localization issues...

Then there's all that 'we fix this later' comments' on this thread, there are too many of them (#236). I thought we used to do it right, do it once.

Gurpartap Singh’s picture

To my best expectations, I find help in _separate_ window the best. I don't want anything to overlap the stuff I'm currently playing with in the current window AND I want to keep the help opened up, even if I move on to some other page in my workspace window.

A modal window popup is more suitable when you have to switch to and fro between content and the stuff in window. Best case I can imagine right now is about Smileys in modal window, where you write some text, insert a smiley, write some more, insert another smiley, and hit Save button (and modal window is gone for good).

Please let's not discuss it here for now, and it's Help window, not popup ;)

merlinofchaos’s picture

Also we lose the ability to display in pop-ups other texts that module help pages. It is badly themeable, and it seems pop up texts can only be in html files (¿are we later creating a text file for each form field we want pop up help for?)

We never had that ability. Now this patch is causing us to lose abilities we never had?

C'mon, Jose. To hear you say it, this patch causes premature baldness, too. You keep complaining about this patch (badly scoped, badly thought out, badly done) yet I've never seen one concrete complaint other than you wish that the help popup weren't part of the patch. Everything else has been vague metaphor and I'm getting tired of you bashing this code without having the courtesy to tell us what code is actually wrong with it. Saying that this code sucks without saying what code, specifically, should not be acceptable, and is not acceptable to me. Tell us what code is wrong or quit complaining about it.

And why is it that we explain the help box, people go "Ok that makes sense" and then a day later they've forgotten and complain again? Repeat ad infinitum.

The next person to complain about the help popup needs to have something concrete to say about it.

The popup for help is NOT a generic popup. It is not MEANT to be a generic popup. It is NOT a javascript modal. That's another patch. Maybe that other patch can subsume this one when it gets in, maybe it won't. Maybe it won't ever get in. Should we postpone this based on that?

JohnAlbin’s picture

  1. The current hook_help() provides worthless help.
  2. This patch is an improvement.

this patch++

IMO, the .info syntax is not a good match for this usage. Comment #242 shows that pretty clearly.

And if we want to standardize on a meta format, we should ditch .info in favor of .ini. The .info files have either simple variable assignments or array assignments. So if we moved to .ini format, arrays could become [sections] and simple assignments would come first and be un-sectioned.

Regarding the comment Dries made about the API (#239 above), maybe we just need some tweaks to the comments to make things clearer?

chx’s picture

@all naysayers: you need popups to be able to view helps on forms without losing the content of the form. That's not complicated. @Jose yea translation is later because this patch is huge enough already so indeed later. @Dries, .help and .info are not the same by any means so why the push the unify them? Just because both support key = value? Give me a break.

I am very much for this patch.

sun’s picture

Status: Reviewed & tested by the community » Needs work

I partially agree with Dries in http://drupal.org/node/299050#comment-1290466

1) That function signature and/or PHPDoc needs work. In general, the PHPDoc for some other functions could use some love, too. Some only have the bare minimum, but are complex enough to deserve a little more explanation (also about required/optional arguments), like:

+/**
+ * Build a tree of help topics.
+ */
+function help_get_tree($topics, $topic_ids, $tree_parents = array(), $max_depth = -1, $depth = 0) {

2) Using .info is not appropriate here (as outlined by others before). However, could we use .ini instead of .help? If we would use .ini, new developers and contributors would immediately understand the expected format of those files. Furthermore, most IDE's and editors already know about syntax highlighting and stuff. So if we do not have a very good reason to add this .help extension confusion, I would appreciate if we could just use .ini instead.

kika’s picture

I do not want to hold the patch back BUT what I feel is that in upcoming months usability task force is trying to reinvent the drupal admin interface http://buytaert.net/mark-boulton-to-help-with-drupal-7 and many flows and UI components (for example notorious "welcome page" and help) in general will definitely be affected. So, perhaps focus on just the help (and long text in general) subsystem first, and deal with UI in a consistent manner later?

Then again, improvements can be done on top of this patch as well. Field API process is another sign of "lets get this in even if this is just halfway solution" philosophy.

> @all naysayers: you need popups to be able to view helps on forms without losing the content of the form.

Nonmodal popup is not the only UI pattern in the planet what allows simultaneous access to the content and the support info in the same time. Current approach is fine but why not explore other options too?

Again, I am not saying let's hold up the patch, both strategies are fine.

BTW, can the live demo be fixed? http://testing.humte.co.uk/head-help/

Pasqualle’s picture

I think we do not have months for this patch, as there are other things to do based on this patch. Everyone who tried it agrees that this is a usability improvement, the task force may try to make it better..

@kika: I do not see, that you wrote any other option for displaying the help, and I do not know (yet) a better option than this patch has..

as I still not see any complaint about the code, please someone roll the patch with the phpdoc requirement (or should be the function name or argument name changed somewhere?) and change the status back to CNR.

dmitrig01’s picture

I agree with Dries (sorry Gurpartap). Why don't we call the file modulename.help, and put it in .info syntax. It is more typing and visually uglier but it is consistent.

redndahead’s picture

Aaaahhh I know I haven't been much help lately with this patch, but for goodness sakes enough feature creep on this patch. My vote is for reroll with phpdoc changes. The .help extension helps one to know what it's there for, but I can go either way. The ini syntax is the normal .ini syntax that we come to expect and the .info is the odd one out. .info files are not standard .ini syntax so I never thought about it being so. Looks like no one has had issues with the code itself minus the popups are satan and translators are going to die of exhaustion using this. I feel the popups issue is a non issue and I think the discussion about translation has proven to me that this system is the best way to move forward.

To provide some help to this patch here is my changes to the first function.

/**
 * This function reads a modules .help file and renders a themed version of each
 * topic. 
*/
function help_view_module($module, $popup = FALSE) {
Gurpartap Singh’s picture

@dmitrig I have stated my reason for the choice already, in #242. And we already call the file name as MODULE_NAME.help. I agree about maintaining consistency, but how much does it _really_ matter here? It's all about giving power to less skilled.

I'll re-roll patch in evening (IST).

webchick’s picture

Here's a summary of the call Dries and I had about this earlier today:

1. While there's a definite desire to see a brand new "contrib documentation squad" formed that magically whisks through all modules and writes extensive, wonderful, multi-page, rich text documentation for them, in all likelihood it's going to be module developers themselves writing 90% of whatever documentation is out there, at least to start. We therefore should think about DX concerns in this patch as they relate to module developers as well as documentation writers. And from a module developer standpoint, it's more convenient to put all of the "registry stuff" in the main .info file rather than in a separate "yet another file format" file that contributes to "file soup" in a typical module directory. And, honestly, if we expect themers to be able to copy/paste/modify three-level-deep nested arrays, it's not too much of a stretch to ask documentation writers to do the same (they're dealing with HTML in the text anyway).

2. The move to HTML-only files means that we can no longer put dynamic PHP code in help text. This seems like a fairly small price to pay for documentation that's actually editable by non-coders without the risk of flaming parse errors, but there might be a need for this in the future.

3. Finally, José (and others) are right, that this patch is trying to do (at least) three things:

a) Introduce an API to act as a replacement for the abomination that is admin/help.
b) Introduce pop-up contextual help so that people don't lose their form data when clicking on it.
c) Introduce a new method of registering and storing large help texts in Drupal.

Combining these three things together result in a patch that's > 120K which effectively renders it un-reviewable. It means none of the individual parts are getting the attention they need and would get if they were split into separate patches.

So. Bottom line. If we want this in core (which we all do), this is what needs to happen:

1. Make this issue *just* about adding the help files and replacing admin/help with an API that can read them. That will allow us to closely scrutinize this code to look for places the API is unclear or can be optimized. This should include a move of help page registration to the modules' .info files using regular .info file sytax and the removal of .help files.

2. We *need* to solve the translation issue. We spent some time at the last Montréal Drupal meetup reviewing this patch, and the people there were flabbergasted and horrified that we were talking about introducing Yet Another Way For Text To Be Translated, and worse, doing so without some sort of web interface to import/export files. And unfortunately, none of the existing arguments really hold much water:
a) I realize that Earl already solicited opinions on this back almost a year ago, but I don't know if word got out to the right people, or if people didn't understand that it was "speak now or forever hold your peace" kind of thing.
b) I don't think it's true that only contributors will translate this text. If you're building a French site for a French client, you do not want any English on the site anywhere. Period. And the more these files are pushed through contextual help, the more likely translators are to be forced to translate these texts.

I am willing to solve this a separate follow-up issue, but we must tread cautiously if we do... one of the primary reasons to push this forward quickly is so that the docs team can get started writing a set of documentation to ship alongside D7. But if we end up having to change the documentation format mid-stream to assuage translators, I'm not sure we've helped ourselves much... :\

3. Move all the stuff dealing with pop-up windows to a separate issue, perhaps merged with the work Tao Starbow is trying to do. While I agree that this is a welcome improvement in contextual help, there's absolutely nothing about that stuff that is required to get a better help system in core and it's just gunking up the patch.

/me dons +1 shield of please don't hit me. :P

Pasqualle’s picture

about the separate followup issue:
people should not be horrified about the translation. The current translation process what we have is simply not good enough. And I personally think this kind of (help) translation is much more natural than what we use in Drupal.

example:
part of the help in cck-6.x is translated to German, and I do not know about any issues complaining about the complexity of the translation process. It is as easy as creating a html file.

I think if you try to translate a book, you will not upload the sentences to some kind of translation server and translate the sentences in shuffled order, but you will translate the book by chapters or page after page..

I do not want to repeat, why it is designed this way, just the link to comments:
#189: #299050-189: Help System - Master Patch
#193: #299050-193: Help System - Master Patch

Aren Cambre’s picture

Concerning both these issues:

While there's a definite desire to see a brand new "contrib documentation squad" formed that magically whisks through all modules and writes extensive, wonderful, multi-page, rich text documentation for them, in all likelihood it's going to be module developers themselves writing 90% of whatever documentation is out there, at least to start.

We *need* to solve the translation issue.

This module treats documentation like repository code, so it's fixed in time and has editing difficulty an order of magnitude more complicated than Wikipedia. This only compounds the above problems.

Drupal documentation should be a living document. Unlike a printed book's edition, living documentation evolves as understanding and knowledge advance. Improvements and corrections are implemented immediately and shouldn't wait until a module's next version comes out.

Online documentation seems like an easy answer. Earlier it was written that online documents are a "nearly unmaintainable mess with no version control and laissez faire." Solutions:

  1. Matching documentation with the module version: Module developers create a copy of the documentation for each major release of the module. So for Views 1.x and Views 2.x, there would be different sets of documents, but Views 2.0, 2.1, 2.2 share the same documents.
  2. Version history of individual pieces of documentation: Already well-supported: http://drupal.org/node/38878/revisions

If you think I am full of garbage, consider this: would Wikipedia work if all edits were checked in through a code repository?

Drupal is web 2.0. Why must Drupal use a strictly web 1.0 model with its own documentation?

Work on this module is a good start on help system plumbing. But the vast majority of long-term effort, and the most objective evaluation of this module's success, will be in documentation quality. That will be difficult with dead documents stuffed in a code repository.

tstoeckler’s picture

We have online documentation and this patch does not intend to change any of that. The improvements you describe might be a great idea, but you should post an issue in the docs section for that. It does not have anything at all to do with this patch.
If you're all for online documentation, fine, no one will stop you.
You must consider, though, that there are people that do not want to rely on an external source for help. If they have a problem with Drupal they Drupal to help them (NOT: drupal.org). And this is what the patch is about: Improving the usability for these people.

Concerning translation: It's been stated before (just like basically everything in this issue...), but this patch is not a translation killer or anything. It was actually formed with translatability in mind: when others suggested to indruce php rendering to help files, that was rejected due to exactly that. Now if a UI is something so important for people to have for this, why not create one. This patch does not kill an existing UI, it simply doesn't provide one, because it is quite reasonable to argue that this is not bitterly needed (although, as I said, one can be of different opinion here.) No one will prevent a follow-up patch that seriously discusses a UI for this...

And about splitting up the patch: Although I'm not opposed to this in general (all I want is this to go in, so help gets better and we can all stop reading the same comments over and over again), what about Dries' patch review earlier, that was basically: RTBC except for the documentation. ???

Gurpartap Singh’s picture

Well, RTBC except a lot of other stuff. Will update the patch in weekend.

webchick’s picture

Just a ping back to confirm I'm eager to get this committed this week. A re-roll would be appreciated. :)

add1sun’s picture

Just to be clear, if you want to commit this patch this week, then you mean to say the only blocker is splitting out the base API and the popups into two patches? Is there anything else considered a blocker for committing at this point. After commit, we can immediately move to the translation issue, or do you want that still addressed now?

webchick’s picture

The blocker is splitting out the patch so that it's reviewable and reviewing it, and having a solid plan going forward for the i18n issues. Doesn't have to be handled in this patch (and in fact, probably shouldn't since it's more than big enough as it is), but if we have a "We are going to solve the i18n issues by doing X thing which will behave in Y way" rather than a "We should really solve the i18n issues someday" I am much more comfortable committing this.

Let's work with José and other translators this week to get a clear plan forward.

starbow’s picture

Patch no longer applies.

redndahead’s picture

Status: Needs work » Needs review
FileSize
115.2 KB
Passed: 10421 passes, 0 fails, 0 exceptions View

Here is my first attempt at stripping out the popup sections. Nothing is taken care of as far as the translation is concerned.

redndahead’s picture

FileSize
115.2 KB
Passed: 10421 passes, 0 fails, 0 exceptions View

forgot to add drupal7 in front of the patch for the bot.

xmacinfo’s picture

Issue tags: +i18n

If we apply this patch, what will be the plan to have it fully translatable later?

What's José’s conclusion on this?

Drupal is wrowing quickly and more and more non-english speakers will hit drupal.org and install Drupal. So we should make sure that both Drupal and drupal.org offers translations for all the help and documentation pages.

Solution 1: Apply English-only new help system, but offer on d.o HTML version of localized documentation packages without any link to the Drupal installation. The HTML could even serve as the source for PDF versions if someone offers to produce them and add them on d.o.

Solution 2: Postpone this patch and reengineer the help system, like José proposed, to load proper translation of the help and documentation.

A lot of people are putting a lot of time on this and from what I've seen in DC, some might abandon working on this, being too time consuming for them.

Webchick and add1sun, where you able to make some progress on this issue and talk to other translators and coders?

webchick’s picture

Issue tags: -i18n

YAY!!! Thank you so much!! :)

Addi, José and Jason from Development Seed, and Jacques/xmacinfo from the Montréal Drupal community discussed this a bit more in DC.

My major hesitation in committing this is that currently there are exactly two people who understand this patch, due to its size: merlinofchaos, and Gurpartap. Since both of them actually coded it, I'm not comfortable committing it until we have a wider understanding in the community about it. This understanding will not happen until the patch is broken up into discrete chunks that can be reviewed on their own.

We discussed how that break up should happen, and it seems like the following makes sense:

a) Pop-ups on contextual help (already split off -- thanks! Probably pick this back up at #87994: Quit clobbering people's work when they click the filter tips link)
b) Allow sticking large chunks of text in HTML files into hook_help(). Basically, solve the fundamental problem of documentation authors having to edit PHP code. José's work could then use this to help centralize large texts that need to be translated.
c) Add in the ability to do hierarchical, topic-based help pages. Then we can analyze whether info files or something else is the best way to do this.
d) Add in search for help text.

I take full responsibility for the mess this patch has become. :( I tried to be lax on the kitten requirements thinking that the enthusiasm the docs team has for these changes could somehow help this patch go through a proper review process despite its size. However, this has not happened, and I know now that I should've really put my foot down much sooner (months ago) about the requirement to break up the patch rather than let it slide. I'm deeply sorry for any feelings I've hurt and any bridges I've burned as a result of my mistake. :(

I know that people have put a lot of time and energy into this and now this demand to break it up probably feels like a kick in the teeth. :( But rest assured, I am not doing this to be an ogre or to make people dance through hoops unnecessarily: I really, really want this in. We just simply cannot have a chunk of code this big go in without a proper review process. Hope you understand...

100,000 karma points to whoever works on this. This series of patches is my current #1 priority for D7.

webchick’s picture

Issue tags: +i18n

Oops.

redndahead’s picture

*sigh* Ok the problem I foresee with this approach is that the way we would allow hook_help to use html is going to be different than what this patch proposes. So, I think whatever patch we come up with will be removed with a later patch. I think b and c fundamentally go together which is what this patch does. I don't even see where search is in this patch. (I could be wrong.)

This patch is large that is correct, but If you look at it most of the patch is made up of the new help files, additional css, and removal of tests for the old help system. By and large the actual functionality of the help system is small.

So let's say we move with the proposal then we have a small patch for b that get's committed. We will then have to work on c. C will be a patch that will be just as large as the current patch, if not larger, because the new help files, the additional css, etc. would have to be added. I really don't foresee this creating smaller patches in the end.

redndahead’s picture

I'll also say I think the reason that their hasn't been a proper review is because coders do not like doing documentation and most documenters do not do code. The people that are reviewing this are people who have an interest in both and I think the people that do have an interest in both are running out of steam.

Another thing about patch b. There is no reason people can't do a file_get_contents on an html file in hook_help. I think the ability to read html files is mainly a documentation of the process.

My comments are not to say I don't understand the need for a better patch review. I applaud you for sticking up for what you feel is best for drupal and that's why d7 is going to be the best ever!

catch’s picture

Status: Needs review » Needs work

I didn't do a massively in-depth review yet, but I agree with rednhead based on what I saw last time 'round. Around 3/4 of the patch is .help, .html and CSS files - none of which is preventing the patch from being reviewed (and the .html are a straight copy/paste from the existing hook_help() so don't need reviewing /at all/).

Here's a .txt - with all the .help and .html stuff removed so we can see exactly what the changes are.
39k patch.
diffstat: 13 files changed, 706 insertions(+), 180 deletions(-).

The actual PHP changes probably come in well under 30k since that still includes remove obsolete tests and adding a bunch of CSS.

This is not a big patch to review, it's just a lack of people actually doing it that's the problem.

Here's the only really important functions introduced (there's a couple of small helper functions beyond that, theme functions renamed/refactored etc.)

help_exists() - centralised function to check if help exists for a module.

help_by_module() - replaces help_main().

help_topic_page() - returns the page for a specific help topic.

help_view_topic() - renders the actual help topic.

help_view_module() - lists help topics for a specific module.

help_get_tree() - shows help topics in a hierarchy

help_get_topic_hierarchy() - shows the hierarchy of help topics for a specific module.

help_get_topic_info() - displays information about a specific topic.

help_parse_ini() - parses .help files

However having looked through these again, I noticed we're missing documentation for the parameters of the vast majority of these functions. i.e.

+/**
+ * Build a hierarchy for a single module's topics.
+ */
+function help_get_topic_hierarchy(&$topics) {

So back to CNW until that gets added.

So the whole system is registering help topics, finding them, and displaying them by module, by topic and individually with navigation to each other.

Popup windows are gone.

Search was never in this patch in the first place.

I don't see any of this which make sense being introduced without the others. If there was a decision to completely gut this patch, kill .help files and use hook_long_text() or similar instead - then it'd have to be split. But that's a decision that needs to be based on the patch as it is - since there's nothing currently to split out that's not dependent on the current architecture - which is the only thing that's controversial here.

redndahead’s picture

/me hugs catch and patiently waits for webchick's response.

I am happy to work with the documentation issues, but I'm going to wait until webhick's response before I start coding. Waking up to a loss of a few hours of coding makes me hesitant this morning. Now I'm off to go see a dentist about my kicked in teeth. ;)

xmacinfo’s picture

Search was never in this patch in the first place.

From what I've heard so far, yes, searching help topics is out of scope for this patch. This can be done later or wait for D8 in my opinion. :-)

redndahead’s picture

FileSize
118.71 KB
Unable to apply patch drupal7-help-299050-2.patch View

OK I'm a sucker. Attached is the patch with the updated documentation.

There is one parameter in help_get_tree() that I wasn't sure about. $tree_parents. That is currently left blank. Hopefully someone can provide some insight.

catch’s picture

So the only real objections put forward here (apart from the size of the patch, which I don't think is a valid objection given the amount of actual code changes are smaller than many, many patches which have gone into core recently with a lot less discussion) is translation of help texts and whether we need separate .help files.

Everything that's added - hierarchical navigation, texts in .html files, basic token replacement - seems entirely necessary to bring help.module even remotely up to date, and can't be split out easily. So let's thrash those two issues out here.

Translation:

The patch will allow help texts to be translatable - just not using the existing locale system. Given the locale system is overloaded and very poorly suited to long texts, IMO that's a good thing. Same as concatenating lines of help text wrapped in t() leads to really, really ugly and hard to maintain help texts in the first place.

I also understand Robert Douglass has been working on something which sucks help topics out of nodes and into CVS and vice versa - which would potentially allow the node translation system to be used for translating the .html files - this seems much more suited to me than locale module (I've not used a pot editor so no idea how they work for long strings, might not be a valid argument when those are used).

While we're on that subject, not all core hook_help() implementations are translatable currently, here's a snippet from node_help():

  if ($arg[0] == 'node' && $arg[1] == 'add' && $arg[2]) {
    $type = node_get_types('type', str_replace('-', '_', $arg[2]));
    return (!empty($type->help) ? '

' . filter_xss_admin($type->help) . '

' : ''); }

To translate this, i18n module has to manually remove the help texts for node types from the table so that it's empty -

  i18n_node_type()
  db_query("UPDATE node_type set help = '' WHERE type = '%s'"

and brings them back via the locale system in it's own hook_help() implementation. I recently did some work on i18n and slightly changed the logic for this - before it was using hook_form_alter() to make $info['help'] an empty string before it was saved. Neither hook_node_type() or hook_form_alter() are are a nice API for translating help texts. If a contrib module does something similar to node.module, without an API which allows you to do direct database queries to gut information from its own storage, or in such a way that hook_form_alter() couldn't be used, then no translation would be possible at all.

The aggregate amount of code required to translate those node type help texts is likely quite a bit more than an implementation of hook_help_topic_info_alter() which could swap in langcode based filenames for help texts (or similar).

If we want a complete re-tooling of our system for displaying and translating long pieces of text, someone should update #365934: Handle big texts / module text files / readable and localizable / get rid of _user_get_text() and get it RTBC. However it's currently at CNW and the last patch posted was over a month ago. Were that to go in first, this patch might need refactoring to take advantage of the API - but that hasn't happened yet. So unless we're going to mark this postponed until that issue is resolved, I don't see what else remains to be done on that front.

The other main objection seems to be about adding .help files rather than using our existing .info files. As I understand it, trying to add hierarchical navigation to our .info files would be really, really ugly. Once again, this is something which needs to be discussed on the patch as it is now - since over 70% of the patch is adding the .html files and registering them in .help - we can't have a small patch which doesn't do this - because then there'd be no way to display the help topics at all, and that's the whole point of the new help system.

For an example of how it might look if we have all the help stuff in our existing .info files, we can take a look at the icons module - which requires themes to declare the icons they use in their .info files:

; $Id$
name = Garland (with icons)
description = Tableless, recolorable, multi-column, fluid width theme (default) with icons.
version = VERSION
core = 6.x
engine = phptemplate
stylesheets[all][] = style.css
stylesheets[print][] = print.css
stylesheets[screen][] = icons.css

; Icons
icons[admin-build-block][16] = icons/16/admin/admin-build-block.png
icons[admin-build-block][24] = icons/24/admin/admin-build-block.png
icons[admin-build-menu][16] = icons/16/admin/admin-build-menu.png
icons[admin-build-menu][24] = icons/24/admin/admin-build-menu.png
icons[admin-build-modules][16] = icons/16/admin/admin-build-modules.png
icons[admin-build-modules][24] = icons/24/admin/admin-build-modules.png
icons[admin-build-themes][16] = icons/16/admin/admin-build-themes.png
icons[admin-build-themes][24] = icons/24/admin/admin-build-themes.png
icons[admin-build][16] = icons/16/admin/admin-build.png
icons[admin-build][24] = icons/24/admin/admin-build.png
icons[admin-content-book][16] = icons/16/admin/admin-content-book.png
icons[admin-content-book][24] = icons/24/admin/admin-content-book.png
icons[admin-content-comment][16] = icons/16/admin/admin-content-comment.png
icons[admin-content-comment][24] = icons/24/admin/admin-content-comment.png
icons[admin-content-node_settings][16] = icons/16/admin/admin-content-node_settings.png
icons[admin-content-node_settings][24] = icons/24/admin/admin-content-node_settings.png
icons[admin-content-rss_publishing][16] = icons/16/admin/admin-content-rss_publishing.png
icons[admin-content-rss_publishing][24] = icons/24/admin/admin-content-rss_publishing.png
icons[admin-content-taxonomy][16] = icons/16/admin/admin-content-taxonomy.png
icons[admin-content-taxonomy][24] = icons/24/admin/admin-content-taxonomy.png
icons[admin-content-types][16] = icons/16/admin/admin-content-types.png
icons[admin-content-types][24] = icons/24/admin/admin-content-types.png
icons[admin-content][16] = icons/16/admin/admin-content.png
icons[admin-content][24] = icons/24/admin/admin-content.png
icons[admin-help][16] = icons/16/admin/admin-help.png
icons[admin-help][24] = icons/24/admin/admin-help.png
icons[admin-reports-access_denied][16] = icons/16/admin/admin-reports-access_denied.png
icons[admin-reports-access_denied][24] = icons/24/admin/admin-reports-access_denied.png
icons[admin-reports-dblog][16] = icons/16/admin/admin-settings-logging.png
icons[admin-reports-dblog][24] = icons/24/admin/admin-settings-logging.png
icons[admin-reports-page_not_found][16] = icons/16/admin/admin-reports-page_not_found.png
icons[admin-reports-page_not_found][24] = icons/24/admin/admin-reports-page_not_found.png
icons[admin-reports-status][16] = icons/16/admin/admin-reports-status.png
icons[admin-reports-status][24] = icons/24/admin/admin-reports-status.png
icons[admin-reports-updates][16] = icons/16/admin/admin-reports-updates.png
icons[admin-reports-updates][24] = icons/24/admin/admin-reports-updates.png
icons[admin-reports][16] = icons/16/admin/admin-reports.png
icons[admin-reports][24] = icons/24/admin/admin-reports.png
icons[admin-settings-actions][16] = icons/16/admin/admin-settings-actions.png
icons[admin-settings-actions][24] = icons/24/admin/admin-settings-actions.png
icons[admin-settings-clean_urls][16] = icons/16/admin/admin-settings-clean_urls.png
icons[admin-settings-clean_urls][24] = icons/24/admin/admin-settings-clean_urls.png
icons[admin-settings-date_time][16] = icons/16/admin/admin-settings-date_time.png
icons[admin-settings-date_time][24] = icons/24/admin/admin-settings-date_time.png
icons[admin-settings-error_reporting][16] = icons/16/admin/admin-settings-error_reporting.png
icons[admin-settings-error_reporting][24] = icons/24/admin/admin-settings-error_reporting.png
icons[admin-settings-file_system][16] = icons/16/admin/admin-settings-file_system.png
icons[admin-settings-file_system][24] = icons/24/admin/admin-settings-file_system.png
icons[admin-settings-filters][16] = icons/16/admin/admin-settings-filters.png
icons[admin-settings-filters][24] = icons/24/admin/admin-settings-filters.png
icons[admin-settings-image_toolkit][16] = icons/16/admin/admin-settings-image_toolkit.png
icons[admin-settings-image_toolkit][24] = icons/24/admin/admin-settings-image_toolkit.png
icons[admin-settings-logging][16] = icons/16/admin/admin-settings-logging.png
icons[admin-settings-logging][24] = icons/24/admin/admin-settings-logging.png
icons[admin-settings-performance][16] = icons/16/admin/admin-settings-performance.png
icons[admin-settings-performance][24] = icons/24/admin/admin-settings-performance.png
icons[admin-settings-site_information][16] = icons/16/admin/admin-settings-site_information.png
icons[admin-settings-site_information][24] = icons/24/admin/admin-settings-site_information.png
icons[admin-settings-site_maintenance][16] = icons/16/admin/admin-settings-site_maintenance.png
icons[admin-settings-site_maintenance][24] = icons/24/admin/admin-settings-site_maintenance.png
icons[admin-settings][16] = icons/16/admin/admin-settings.png
icons[admin-settings][24] = icons/24/admin/admin-settings.png
icons[admin-user-permissions][16] = icons/16/admin/admin-user-permissions.png
icons[admin-user-permissions][24] = icons/24/admin/admin-user-permissions.png
icons[admin-user-roles][16] = icons/16/admin/admin-user-roles.png
icons[admin-user-roles][24] = icons/24/admin/admin-user-roles.png
icons[admin-user-rules][16] = icons/16/admin/admin-user-rules.png
icons[admin-user-rules][24] = icons/24/admin/admin-user-rules.png
icons[admin-user-settings][16] = icons/16/admin/admin-user-settings.png
icons[admin-user-settings][24] = icons/24/admin/admin-user-settings.png
icons[admin-user-user][16] = icons/16/admin/admin-user.png
icons[admin-user-user][24] = icons/24/admin/admin-user.png
icons[admin-user][16] = icons/16/admin/admin-user.png
icons[admin-user][24] = icons/24/admin/admin-user.png
icons[admin][16] = icons/16/admin/admin.png
icons[admin][24] = icons/24/admin/admin.png
icons[help][24] = icons/24/misc/help.png
icons[message-error][24] = icons/24/status/status-error.png
icons[message-status][24] = icons/24/status/status-info.png
icons[message-warning][24] = icons/24/status/status-warning.png
icons[node-add-book][24] = icons/24/admin/admin-content-book.png
icons[node-add-story][24] = icons/24/admin/node-add-story.png
icons[node-add][24] = icons/24/admin/node-add.png
icons[user][24] = icons/24/admin/user.png

IMO, keeping help text registration out of the main. info files is a good thing, and avoiding having two different files with .info extensions doing two different things is also a good thing.

More nitpicks on the current patch while I'm here:

In help_view_topic()

// Change 'topic:' to the URL for another help topic.
+    $output = preg_replace('/\[topic:([^"]+)\]/', strtr(url('admin/help/$1'), array('%24' => '$')), $output);
+
+    global $base_path;
+
[snip - lots of preg_replace()]
+
+      $output = _filter_autop($output);
+    }
+

Thinking this section could go into a helper function which takes and returns $output. Something like help_process_topic($output, $line_breaks); - just to make that whole function a bit more readable.

Also I don't like the direct calls to _filter_autop() - we should either make that a public function with a more meaningful name, or it looks like the same thing could be done with filter_filter('process', 1, -1, $output);

redndahead’s picture

FileSize
118.75 KB
Passed: 10421 passes, 0 fails, 0 exceptions View

This patch changes _filter_autop to filter_filter. Double checked and it is a valid change.

As far as help_process_topic goes there are some things I wasn't sure about. To move those into it's own function we would also need to get $file_info and $info variables. So here are my ideas.

function help_process_topic($process_type, $output, $options = array()) {
   switch($process_type) {
     case 'url':

     case 'path':

     case 'trans_path':

     case 'base_url':

     case 'line_break':

   }
}

This would require us to call the function for each filter.

or

function help_process_topic($process_types, $output, $options = array()) {
  if (in_array('url', $process_types)) {
    
  }

  if (in_array('path', $process_types)) {

  }

  if (in_array('trans_path', $process_types)) {

  }

  if (in_array('base_url', $process_types)) {

  }

  if (in_array('line_break', $process_types)) {

  }

  return $output;
}

This would take an array of processing types and then process the output according to what's in the array. The $options variable is where you would add the $file_info['path'] etc. as

array(
  'help_path' => $info['path'], 
  'file_path' => $file_info['path'],
);
redndahead’s picture

Thought I would add @TODO's so we can keep track.

TODO:

  • Finish documentation for help_get_tree()
  • Create help_process_topic()
sun’s picture

Wow - please don't squeeze any changes for the filter system or filter.module into this patch. Changing _filter_autop() to filter_filter() is _definitely_ outside of the scope of this issue and has zero changes to be committed.

redndahead’s picture

This is not a change in the filter system. filter_filter already exists and all we did was change this patch from using the private function to a public function. in help.admin.inc and help.test

catch’s picture

sun - we're calling filter_filter() directly instead of _filter_op() directly not changing anything in filter.module:
http://api.drupal.org/api/function/filter_filter/7
http://api.drupal.org/api/function/_filter_autop/

Cleaning up filter module is definitely out of scope here, there's a nice issue about that though #258939: Filter system revamp. I don't really like either of the private function call or filter_filter's horrible parameters, but it shouldn't hold this one up.

redndahead, help_process_topic() looks a bit more complex than I'd imagined - seems like a good argument to leave it exactly how it is in the current patch. Sorry for the dead end.

Which means:

TODO: Finish documentation for help_get_tree()

Frando’s picture

FileSize
74.76 KB
Passed: 10551 passes, 0 fails, 0 exceptions View
40.21 KB
Failed: 10377 passes, 2 fails, 2 exceptions View

For easier review and thanks to git, attached are two patches to make this issue easier to review. Both patches combined are exactly the same as #279.
help_code.patch contains all code related changes, while help_helpfiles.patch contains the added .help and .html files. So if you want to review this patch, just review help_code.patch.

Now, help_code.patch weights 41k and has a diffstat of 13 files changed, 834 insertions(+), 180 deletions(-), which is OK to review, IMO.

Frando’s picture

review of help_code.patch from #284 (which is the same as #279):

function help_by_module():
* docblock should be improved.

function help_l():
* what is this good for? In the patch it's nothing but a wrapper
around l(). If it's needed for the popups part (later patch), I'd be OK with
leaving it in for now to. Otherwise or if the purpose isn't clear, remove it.

function help_topic_page():

+ $output = help_view_topic($module, $topic);
it would be cleaner to call this only if $topic is actually set. So doing an
if($topic) instead of if($output).

  +  $breadcrumb[] = help_l(t('Help'), "admin/help");
  +
  +  $breadcrumb[] = help_l(t('Administer'), 'admin');
  +  $breadcrumb[] = l(t('Home'), '');

why would you not just use l() here?

function help_get_tree():
* definitely needs better comments (inline). Took a bit to actually grasp it.

function help_view_topic
* The character encoding for the next and previous links is broken.

* + $navigation = '<div class="help-navigation clear-block>"';
this just doesn't look right when not in a theme function. So better add a theme function for the navigation, previous/next links and the wrapper div.

Otherwise, the patch looks really good and is a definite improvement about what we have currently. The whole .info file parsing, file selection and rendering and tree generation code looks quite good to me. I'd be totally +1 to commiting this patch after another review or something. It's really in a good shape IMO.

redndahead’s picture

help_l() - Ahh crap I was hoping I could just get by with this. The reason I left it in there, knowing that it wasn't doing anything, was because in a future patch we would be adding popups. These popups will use these functions. So instead of stripping out all the help_l() functions just to put it back in in another patch I left it there knowing we were going to change it a little in another patch.

I'll look at the rest.

TODO:

  • help_by_module() documentation
  • help_get_tree() documentation
  • Fix character encoding for help_view_topic
  • Theme function for help_view_topic?
  • Finish Documentation for help_get_tree()
Gurpartap Singh’s picture

Wait redndahead. My cannon ball will be hitting in the next hour. :P

Frando’s picture

FileSize
74.76 KB
Failed: 10609 passes, 1 fail, 0 exceptions View
41.53 KB
Failed: Failed to apply patch. View

This patch
* improves the documentation a bit here and there
* some code style cleanup
* fixed the character encoding foobar
* adds a theme function for help_view_topic()

Now holding off for Gurpartap's cannon ball.

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
115.74 KB
Failed: Failed to apply patch. View
  • Added help for field module
  • Removed popup related CSS styles
  • Removed popup related information from help module's help itself.
  • Added comments where necessary.
  • And other nitpick changes I can't recall right now.

Use patch -p1 < drupal7-help-system_299050-289.patch (instead of the usual -p0) to apply this patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
114.95 KB
Unable to apply patch drupal7-help-system_299050-291.patch View

This one should work with 0 value for -p option.

Frando’s picture

Status: Needs review » Needs work

The latest patch does not include some of my changes in #288, e.g. the theme function for and changes to help_view_topic are missing.

Also, the patch changes a nonexistent file (modules/fields/help/fields.help). It seems you first copied over an existing .help file and accidentely added it to the index.

Frando’s picture

FileSize
74.9 KB
Passed: 10551 passes, 0 fails, 0 exceptions View
40.91 KB
Failed: Failed to apply patch. View

Attached patch merges everything together again. Was mostly just the theme function and a few strings from my patch in #288. I hope this is correct now.

Edit: Sorry, just noticed I forgot --no-prefix, so please apply with patch -p1, whoever is going to reroll next.

redndahead’s picture

Frando any chance of you rerolling it with -p0 and moving to CNR. I don't think test bot will like it otherwise.

Gurpartap Singh’s picture

Status: Needs work » Needs review
FileSize
115.24 KB
Passed: 10421 passes, 0 fails, 0 exceptions View

I had included the theme function already, but hadn't implemented yet. Most of the code was buggy, I mean horribly buggy that I didn't want to look at it this night. But I have now fixed it and included in the attached patch.

kika’s picture

Is it very hard to remove help_l() ? Looks ugly special case, why not build generic popup support into l() itself in the future?

redndahead’s picture

Kika,

I have looked this over and I really don't think there is a better way to handle popups. This function will check to see if it is a popup and create the link with ?popup if it is. If we remove this function we would have to check every time we use l() which could get messy. There is no benefit to adding this functionality to the l() function as this functionality is specific to help.

starbow’s picture

The way I handle this over at Popup API (and the propsed popbox patch) is to include a class on the link I want to hijack, then have a js behavior that attaches the open function to all links with that class. Then there is no need to have a distinct function or to change l().

starbow’s picture

the other benefit is you can hae a css rule for that class and style the link.

Gurpartap Singh’s picture

^ Then, it won't render popup styled pages without javascript. You need a server side solution for this. Current help_l() can probably be moved into l() itself in the popup issue, if it adopts this method, which it should.

redndahead’s picture

Again the problem I see is are we going to have every popup page append ?popup to the end of the url for all of drupal? The reason for help_l is to append the ?popup not to necessarily actually make it popup.

Pages