Dries doesn't like the revision overview screen.

1) there is a numbered column in the front of the table which he says does not make sense.

2) he does not like the way the log is output either.

3) he is not sure the title makes much sense.

I'd like to invite everbody to discuss this and I will rework the screen to match whatever consensus is reached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

killes@www.drop.org’s picture

FileSize
25.05 KB

here is a screenshot, feel free to annotate.

m3avrck’s picture

Where does the 'initial version' etc stuff come from, is that the log?

1. numbers make sense, revision 1, revision 2, etc ... maybe perhaps prefixing 'rev 1', 'rev 2' would make more sense?

2. looks ok to me

3. title makes sense because you can change the title revision to revision, perhaps a better screen shot showing this would have it make more sense?

I'm indifferent to revising this but I'd like to see what other people think too.

killes@www.drop.org’s picture

yes, initial version etc is the log. A suggestion was to change the numbers to the date.

Stefan Nagtegaal’s picture

I'll post a UI-review tomorrow night...

factoryjoe’s picture

So is there a screenshot somewhere I can see?

factoryjoe’s picture

FileSize
14.73 KB

Whoops, hadn't refreshed before I commented. Check out the way Writeboard handles revisions *on the node* itself.

killes@www.drop.org’s picture

Dries, how do you want me to proceed?

Zen’s picture

FileSize
10.04 KB

Screenshot attached. I've done away with "title", and replaced "(current)" with an asterisk. Not sure about the latter though..

Cheers,
-K

m3avrck’s picture

I would switch that last one around, putting on the top line:

Revision 2005-12-12        View Set Delete
Log message 08              AuthorName

Just changing the focus to the revision and date with accompanying actions and putting the log message and author name as secondary (which they really are).

m3avrck’s picture

Also, I wouldn't make each row in the table seperated by lines (too confusing) but rather only use lines to seperate each revision (so every 2 rows).

killes@www.drop.org’s picture

I'd like to point out that the log message can be longer than any of the examples used in the mockups. this should be considered.

m3avrck’s picture

Right, so if we use the approach above I mentioned, if log messages are a few lines long and they wrap, it won't interfere with selecting revisions, especially if each revision is seperated instead of row, it will make the readability much better, IMO.

Dries’s picture

The screenshots look ugly/complex IMO. Why do we want to make multi-row table entries? It complicates things. Can someone try one where everything is on one line (of course, the message is allowed to wrap)?

The "view set active delete" is somewhat confusing as well, because there is no clear separation between different options (because the links are not underlined). Maybe 'set active' should become 'activate'?

eaton’s picture

If a given revision is set to 'active' will the revisions AFTER it be preserved, or discarded?

"Rollback" or "Restore" might be easier to grasp titles for the link than "set active." That's what a user is trying to do, after all -- restore the contents of an older revision.

m3avrck’s picture

FileSize
91.33 KB

@eaton yes older revisions are not discarded unless they are specifically deleted.

Here is a new screen where I put my earlier ideas into something visually pleasing.

A few caveats that you guys might come up with: to view older revision, click on the date (duh!) as this is generally the case, the clears up the operations, we can obviously add a 'view' link but that is too much clutter IMO.

Additionally, as Eaton suggested, 'set active' isn't usually used when working with revisions, 'rollback' or 'revert' tend to be better choices so I have gone ahead with that.

This really cleans things up and makes it easier to read.

@Dries notice how long log messages could be, this makes it easier to view if they wrap underneath instead of on one line. A lot of themes are fixed width and not too wide and having log messages on the same long could produce some extremely short length lines (1 or 2 words) having it below gives it more room to breath.

Zen’s picture

@m3avrck's layout. A lot cleaner and I like the fact that "view" is gone and the "by user" keeps things a lot more concise.

+1

Cheers :)
-K

m3avrck’s picture

FileSize
98.49 KB

Slightly better one based on comments from IRC.

Zen’s picture

Cool :) Only thing is that the duplicate asterisk seems out of place. Perhaps the second one can go *poof*?

Also, I just realised that "Revert" (as Eaton mentioned) might be slightly inaccurate/misleading here, as newer revisions are still retained. It also implies "going back to something in the past" while that might not always be the case here. As Dries suggested, "Activate" might be more suitable..

But +1 except for these nit-picks :)
-K

Dries’s picture

The latest screenshots look a lot nicer! :)

m3avrck’s picture

Ok one last revision based on comments from above and IRC and off of how wikipedia works

Now, just to be clear, revisions should go in reverse chronological order (newest first), this is now clear in the screen shot.

Secondly, we should create a new CSS class, called .revision-active. Reason for this, is that many themes make use of alternating table row colors. With this new change, that is still supported, however to mark the current revision as active, we need another class so that this can stand out.

Thirdly, the 'revert' issue. By defintion:

revert: go back to a previous state;

Now revisions work like this: I create revision 1, someone else doesn't like it, so they create revision 2, someone else creates revision 3 and so forth. As each revision is created, it is marked as 'current' as generally revisions mean improvements. So if revision 3 is current and we want to go back to revision 1, we revert to revision 1.

Of course, revision 1 is the oldest, but it is now the current revision. If we want revision 3 to be current, we go back to revision 3 (because despite it having the newest creation date, it is still an older revision now because revision 1 is current). Hence, we revert back to revision 3.

That is why I suggest staying with 'revert' I see it come up the most often when dealing with revisions.

As a note, I natively speak English but I'm by far, no English major ;-)

m3avrck’s picture

FileSize
98.34 KB

forgot screen shot.

killes@www.drop.org’s picture

Just a remark: If you revert to an old revision, there will a new revision be created which is a copy of the old one.

Zen’s picture

@m3avrck: Neat :) I'm OK with either, but thought Activate might be a more neutral expression :)

@killes: Ah, I hadn't noticed that :) Perhaps an anchor link [In the copied revision's log message] to the original revision might be handy?
OT - I also noticed that the watchdog messages don't seem to indicate any of the revision activity - they just say "page update ..".

Cheers :)
-K

m3avrck’s picture

Status: Active » Needs review
FileSize
5.99 KB

Ok here's a patch to fix this, it accomplishes the following:

1. Cleans up the display to look like screen shot in #21
2. Changes all references from 'rollback' to 'revert' to be consisent with screen shot
3. Reverses node_revisions query for DESC instead of ASC (newest revisions should be at top)

m3avrck’s picture

FileSize
9.12 KB

Here's a screen of what this looks like in bluemarine theme.

Dries’s picture

Status: Needs review » Needs work

Close but no cigar. ;-)

"Log message:" can't be translated.

t(' by ') might not be a good idea. Probably better to use t('%date by %author', ...) or something equivalent. You might want to check how other 'by's are translated elsewhere.

There are some instances of 'rollback' left:
'%title has been rolled back to the revision from %revision-date'

Steven’s picture

Why do we need "Log Message:" in each entry at all? Isn't it clear from context? It just seems like clutter to me.

m3avrck’s picture

FileSize
7.6 KB

Dries, good call! It was late last night when I did the patch, here's a better one. Fixed all instances of the term 'roll' so everything is consistent now. Fixed the translatable 'by' string. And as Steve suggested, I removed the 'Log message:' all together. Take a look at the screen shot and compare to the one in comment in #25 ... whichever looks better we'll go with.

m3avrck’s picture

FileSize
8.46 KB

And the screen with 'Log message' removed.

m3avrck’s picture

Status: Needs work » Needs review
Dries’s picture

+      $row[] = t('%date by %username', array('%date' => l(format_date($revision->timestamp, 'small'), "node/$node->nid/revisions/$revision->vid/view"), '%username' => theme('username', $revision)))
+               . (($revision->log != '') ? '<p class="revision-log">'. $revision->log .'</p>' : '');

Displaying raw data ($revision->log) is vulnerable to XSS attacks, or is the log field checked/escaped elsewhere?

m3avrck’s picture

FileSize
8.07 KB

Dries, you're right. The code was that way before I made my patch, I've updated it accordingly. I also noticed that $node->log was wrongly being output in another spot too and I have fixed that in this updated patch.

Dries’s picture

Status: Needs review » Fixed

Committed to HEAD. Thanks.

Zen’s picture

Status: Fixed » Needs review

I really think that losing the log field is a _bad_ idea. If I have a 100 revisions, I will be totally lost without the log field. I personally already have pages with about 20 revisions in them, where this would prove invaluable. Sifting through them manually to find out which revision to revert to is not something I'm looking forward to.

Perhaps a compromise could be to just truncate the log message at 128 chars or something with a trailing ellipsis appended :)

I am also not too keen on the fact that copies are being made after each revert - it just seems very wasteful. There should be a more efficient solution for preserving the revision history of a node.. But, for the purposes of this issue, would it be a good idea to link the copy to the original via an anchor link? Again, this is to make things easier when the number of revisions listed is large.

Hope that made sense :)
-K

m3avrck’s picture

Status: Needs review » Fixed

Zen, the log message is *not* gone at all, it is still there. We were refering to the *actual* text: 'Log message' preceeding the actual log message.

As for why copies of revisions are made, see my other thread: http://drupal.org/node/45071 ... I actually *now* agree that copies make more sense, as they show the flow of work on a node and you can easily diff changes.

Zen’s picture

Ah right - apologies for jumping the gun there. The "Copy of revision..." threw me off.

Diffs - ok, thanks - more reading :)

Thanks,
-K

m3avrck’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
2.72 KB

Hmm, looks like that new CSS class to highlight the current revision as yellow didn't go in.

Also, realized that check_plain() was wrong. Misunderstood chx, should have used filter_xss() so that any HTML in the log message can actually be rendered.

New patch fixes both of these.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks again.

Anonymous’s picture

Status: Fixed » Closed (fixed)