Use menu local tasks for revision links and actions.

Hmm, I see that you include template files for theming, but it still seems to me like wrong approach to include it in content.

I think this is bug (bad design). Links are actions or navigation, depending on how you treat it, and not content. One shouldn't put anything in content, that is not content itself.

There is menu local task facility. Revisioning should use that by default. If it worked so from the start, we wouldn't have problems such as #489146: Output Links in the $tabs variable.

Just because people don't complain we shouldn't use bad design. People are used to use generic solution. For example, if they use simple templates, links in content is not problem, because they are printed right below node title and tabs. Yes, it works, but it's ugly and it works only for generic case. And when there is some specific case, you gonna have problems, like with Panels. And even with simple case there can be problems, like it was with displaying revision links in teasers. That's what happens when bad design brings new problems.
In such complex and tightly integrated system, like Drupal is, one should design carefully and try not to implement hacks (inserting links in content is very very ugly hack), to ensure that bad design won't bring any problems in future.
There is clear distinction between navigation and content. Every developer or system architect (not just Drupal) must be used to this concept, it is common IT practice.
When other Drupal module wants to output content, it expects content and only content. When you output content with links, you actually break API and break expected behavior. That's what API's for: you don't reinvent wheels, you design carefully and follow the API, and then you can be sure that any other module implementing same API won't have any problems with your module.

Personally, sometimes I use some hacks. However, when doing it, I ensure that there is no clear way to do what the hack does. This is clearly not the case with Revisioning module.
You could use menu local tasks, with even 2 levels of tabs!
Also you could use node $links array
Also you could implement block, which contained revision links
For Panels, one could implement Panel Pane (which is relative to block), which contained revision links

So many clear ways and revisioning module goes the dirty one, it's sad.

Sorry if it looked like ranting. I was just arguing against bad design. As for

why not do something useful and suggest a patch for consideration by the community

I think providing bug report and pointing to design flaws is useful enough. Ofcourse, if you struggle for quality code, and fixing bugs. And it is kind of rude to tell people stuff like this, like bug report is not useful. Not everyone can, or will, or have the time to inspect your code or even program.
I will try to make a patch, but can't promise anything.

Don't worry, patch is almost ready

I found one small problem: it seems, that menu system won't let me have revision local tasks as secondary tabs for node, because there is second variable %vid in the path, in addition to %node variable. So far revision actions work only as primary tabs for selected revision. That is ofcourse not big problem, just one more click to get to the parent node. I'll look into deeper, maybe someone have ideas ?

Hmm, tricky part is to make "revisions" or "revisions/%" to work as parent for secondary tabs. Trying to grok if _to_arg() can help with it...

Well, you can't really expect from menu system exactly same behavior as links in content. For example, revision tasks that deal with specific revision won't appear until you choose revision (i.e. on path like "node/123"). That's why they are called LOCAL tasks, they are local to their context, and with path "node/123" revision context is not available yet. Only part that could be possible, I think, is what I do now - make menu system treat revision tasks as secondary level tasks of nodes, so on path like "node/123/revisions/2/view" both revision and node tasks could appear.

We could use other means to display links, besides menu system, for example custom block. I'm fine with different approaches, as long as they don't break API. Actually, I am trying to use Revisioning module in my project that uses Panels, so either panel pane or block will work for me very well.

It seems like I made it to work. Now revision tasks are secondary level tasks. For that I had to move revision list page from "node/123/revisions" to "node/123/revisions/all". Also "node/%node/revisions/%vid" is now dynamic local task of node, that serves as parent for secondary level of tasks. When %vid is absent, for example on path "node/123" I had to fill something in, so I decided to use current revision. So on path "node/123" where node 123 has current revision 111 there is local task "node/123/revisions/111" called "Revision". Unfortunatly we can't change menu titles dynamically, so we have to figure out proper name for that local task that will work in all cases.
You can ask, why did I have to make new tab with "node/%node/revision/%vid" ? Well, for 2 reasons: there must be parent item for secondary tabs to work, and "single revision" context must be separate from "multiple revisions" context.

In summary, we have 2 new tabs:

1. "All revisions" tab

This simply contains list of all revisions for node. This was previously called "Revisions". We can call it "all revisions" or "list of revisions". I decided to drop "Revisions" name because it was not clear enough, when displayed near next new tab, which is described below.

2. "Revision" tab

On global node page, such as "node/123" it points to current revision of node, and on specific revision page, such as "node/123/revisions/12/view" it points to revision 12 of node 123.

Any ideas for good names ? How about "This revision" instead of "Revision" ? This would properly describe it's behavior, I think.

One more question. Currently in my patch because of it's path ("node/%node/unpublish") "unpublish current revision" action is local task of node, not revision, with title "Unpublish" (in other words, it's primary tab with name "Unpublish").
Do we actually need it as local task of revision (secondary tab) ? In that case I will need to move it to path "node/%node/revisions/%/unpublish".

Hmm, I think I will move it, because before it was printed in content links, which are currently replaced with secondary tabs, and we want this patch to be consistent with pre-patch behavior.

I decided to keep "node/%node/revisions" menu tab, because it's set up by core and modules may depend on it. Question about the name stands still though...

Some more news:
1) Thanks to badly documented feature of menu system "tab_parent" we don't need "This revision" tab! So we keep "Revisions" tab untouched, and use it as parent for our revision local tasks.
2) I can't read! Contrary to what I said in #16, actually menu system allows us to have dynamic menu titles, thanks to "title callback". That means together with %vid we can also change titles dynamically, to indicate target of local task, for example when %vid is absent. And that is just great from the point of UX.

We probably need some menu system guru to review this patch. It uses some badly documented menu features.

Patch summary:

• Removed template files that are now obsolete.
• Removed .css file that is now obsolete.
• Removed theming code for links inserted in content, that is now obsolete.
• "Revisions" tab is now parent for all revision tasks.
• All revision-specific menu callbacks are now local tasks, rendered as secondary tabs.

@midkemia,
This patch doesn't add new primary tabs, only secondary ones, as children of "Revisions" tab that is already present. Please read patch summary carefully, or better try it out ;-)
If one has too many tabs he can reduce font size for their titles so they look nicer. Also some themes display tabs in a way you would never imagine.

@RdeBoer

having tabs that apply to a selected revision appearing under the "Revisions" tab is a bit clumsy-looking

Well, before I had standalone "This revision" tab as parent, but it was more not better, basically "This revision" tab served as logical and visual parent, but was useless itself and took up valuable primary tab space. We can fix it with proper title, like rename "Revisions" to "Revision tasks".

You were a bit over-zealous with deleting code as the patch removes a few crucial links ("Delete all revisons", "Unpublish current revision" above the revisions summary overview (i.e. node/%/revisions). These are also candidates to be transformed to secondary local tasks and I guess that would be the next step

OK I will address it in next version of patch.

We do have one more issue. The "Delete revision" local task needs to have a different URL behind it, depending on a condition: if the revision shown is the only one, then we need to delete the NODE, whereas, if it isn't we need to delete the revision. May be should point to an altogether different link which takes us to a function that does the logic and forwards to to correct operation?

If deleting only revision of a node is bad - then we must do it in single callback function which makes sure we don't break anything. If deleting only revision of node is ok, then it would work both ways. Actually, I never tried to delete only revision of a node, what a shame...

That is general problem with revisions. We always chose whether our actions are revision-centric or node-centric, and sometimes decision is hard. We have also "edit node" and "edit current revision" and difference is not clear.
I have one idea related to this. Since we are reworking Revisioning callbacks anyway, we can also change how node local tasks work. For example, before this patch primary "edit" tab was hidden (set to be simple callback instead of menu local task) to prevent unconcious editions. We can continue our effort to make tasks more revision-centric, instead of node-centric. How about these steps:

• We rename "View current" tab to "Current revision" (since it points to "node/%node" which always points to current revision)
• We use it as parent for our current revision tasks
• We make new secondary tabs (under "Current revision"): "view current" local task, which is default local task, "edit current" which points to "node/%node/revisions/%current_vid/edit

We would then just need to be sure that our callback functions are smart enough to deal with edge cases, for example when node has only 1 revision.

Actually it's very very big question of usability and user experience. Voice your opinions!

Ok I will wait till you check this patch in. Actually it's already usable enough so we can wait some time to gather feedback from userbase.

I'm a little confused as to using the "View current" tab as the root for the secondary local tasks that we have at the moment as these operate not on the "current" revision, but on the "selected" revision

I meant to use "view current" as root not for all secondary local tasks but only for tasks that operate on current (not selected!) revision. That would save us some clicks, because these tabs then would display on global node page ("node/%node") and user wouldn't have to go to "Revisions" tabs. But then, if we rename "Revisions" tab to "Revision tasks", it will be again messy because current revision task will be outside of "Revision tasks"...damn :) Maybe it was bad idea.

Would renaming "Revisions" to "Revision tasks" be intuitive when this tab is used for two things at the moment: for revision tasks as well as the revision summary(i.e. while looking at a selected revision, clicking the Revisions tab another time is the same as clicking "List revisions")

Why would someone want to click on "Revision tasks" while looking at selected revision ? He would have that tab already open (active), if your theme renders tab this way, ofcourse. Zen theme, for example, does it. And it's illogical to click on tab that is already active.
You always need to have default local tab which inherits it's path from the parent. So we either put one more parent tab at primary level either leave "revisions" as parent. I liked keeping "revisions" cause primary tab space is valuable. Ofcourse we all would be happy if revision tasks could be tertiary instead of secondary, and we could have hierarchy like "revisions -> selected revision -> selected revision task" but unfortunatly Drupal gives us only 2 levels of hierarchy that are displayed at the same time.
I don't see big problem with "Revision tasks", but it's me and I'm fine with "Revisions" title too. We need to hear user opinion about it probably.

Let's do some mockups, for better understanding. I will edit this post for future clarifications. Now we have following hierarchy (order of items is not accurate):

Node ------> View current
             |
             Edit (hidden)
             | 
             Delete (?)
             |
             Revisions ------ List revisions
                              |
                              Edit revision (when specific revision is selected)
                              |
                              Publish revision (when specific revision is selected)
                              |
                              Unpublish revision (when specific revision is selected)
                              |
                              Revert to this revision (when specific revision is selected)
                              |
                              Compare to current (when specific revision is selected)
                              | 
                              Delete revisions (when specific revision is selected)

Some new ideas here.

1. We can't afford to explain core limitations to every user (anyone who has some knowledge of UX can confirm that).

If we settle for overcome Drupal core limitations we need to allow all simple actions user would think about as logical. And deleting current revision is logical. So "Delete" action is the first candidate. More to come.

2. About "Delete" action: we need new type of action, that would apply differently depending on context and would allow any kind of deletion

Deleting current and selected revision SHOULD be available. we just need function that is smart enough: if current is the only revision, delete node, if there are multiple revisions, revert node to other revision first (probably to previous revision), then delete selected revision. Probably a dialog form needed: "Are you sure you want to delete current revision ? Yes/No + List of revisions with radios to select revision to revert to".

That might be true, but for experienced users who are used to always having an "Edit" tab for each node, removing it becomes very confusing—they might not know to click on the "Revisions" tab to edit the node, especially in the case where the node only has 1 revision.

I agree, this is bad behavior. We should present to user what he expects to see. And user expects to see edit and delete buttons. I think instead of hiding tabs we should focus on making actions more smart so they do what user expects them to do. For example when user visits "node/1" he expects to be able to edit what he currently sees, - current revision of node 1. He probably doesn't care if there are other revisions, and if he does, we display warning anyway, that this is revision number XX.
Note that we can also change tab title according to it's action on current page. So this gives us additional flexibility

Its solution to deletion of the current revision (if it wasn't the only one) was to NOT offer the delete option, which forced the user to first select another revision to be "current" (published or not), so that the previously current revision could be deleted. The whole dialog thing (first confirming the delete, then offering a screen with radio buttons) seems a bit heavy-weight to me, both from the user's point of view as well as the programming point of view.

Well it's not obvious to user he has to choose revision first. OTOH when he has button that is present and visible but involves dialog, it's much better. And programming heavy-weight should not be the problem if it helps with UI heavy weight, which is worse imo.

Yes, because the dynamics have changed, we need to rethink the suppression of the Edit tab. I'm happy to run with it, although it probably should be renamed "Edit current".

Yep, that would be consistent with "View current"

FYI: Merlin, the man who is author of Views and Panels modules, made CTools module which contains lot of helper functions. For example, he added facility to allow dynamic menu tabs inclusion with very very simple api. His functions don't mess with menu system but instead add tabs dynamicall via theming system (for that CTools has theme override for theme_menu_local_tasks() ).
What I done with dynamic secondary local tasks of revisions was a bit hackish. We could add CTools as dependancy (which is not problem generally - Merlin's modules are always of good quality and have decent support) and use CTools' function to output tabs. Also CTools tab function could be used to add more tabs if needed on the fly. For anyone interested, look at "includes/menu.inc" of CTools. Downside of that would be the following: themes that implement their own theme_menu_local_tasks() would need additional modifications (see comments in "includes/menu.inc" of CTools).