[PATCH] "are you sure you want to delete this page?" question needs more details

[team@…]

When I'm trying to delete a page from Wiki, I see the following message:

Delete MyPageName
Are you sure you want to completely delete this page? 
This is an irreversible operation.

Would be much better to see this text:

Delete MyPageName
Are you sure you want to completely delete this page? 
This is an irreversible operation (23 revisions since 3-Jan-2010).

[Christian Boos]

… given the rate at which you're making (nice) suggestions, I think you should now also seriously start to consider bringing directly the changes themselves on the table in form of patches. See TracDev/SubmittingPatches if you're not yet familiar with the process.

Especially for the kind of change suggested here, it would be for us just a matter of a quick review and applying the change. Could be much more efficient than waiting for us to eventually implement the changes… Plus you'd be able to test how your proposed change would look and feel like and the suggestion could come in a more refined form.

[team@…]

will do, once I finish reading the book about Python :)

[Christian Boos]

2 months is more than enough to learn Python, no?

Anyway, this was a good suggestion and we'll eventually do it by ourselves when time permits.

[Mikael Relbe]

No, 2 months is a very short time for some of us ;) I'll do this…

[erne.castro@…]

Patch for current 0.12-stable to add more info on the page delete confirmation.

[erne.castro@…]

I just playaround a bit with this and came out with this patch.

I'm sure its not the perfect solution, but is just my five cents.

Please don't be cruel… this is my first patch submitted! :)

[Christian Boos]

Not bad for a first patch ;-)

I made a few follow-up changes,

• the <a href="${href.wiki(...)}">... and last one ${dateinfo(last_date)} ago. line was a bit long
• the wording could be improved: instead of e.g. 5 revisions, first one 1 month ago and last one 1 day ago) I prefer to display: Removed all 5 revisions of the page, which was created 1 month ago and last modified 1 day ago. earlier in the page (this just a detail, of course, and you may disagree ;-) )
• the message was lacking i18n support (well, that link doesn't show much info for now, that will be hopefully be expanded one day)
• the Python code is OK, but after duplicating the loop, you could have continued and simplified the code a bit

As an example of how could things be polished further, I add a few more patches on top of yours. I hope this will help you to get started!

[Christian Boos]

some improvements to t9220-page-delete-more-info-0.12-stable-r10138.patch​ (1/3)

[Christian Boos]

applies on top of wiki_delete_improvements.patch​ (2/3)

[Christian Boos]

applies on top of no-loop.patch​ (3/3)

[Mikael Relbe]

Argh… I was also producing a patch today… I'm supplying it anyway, perhaps it can create some inspiration: t9220-mrelbe-r10140.diff​

[Mikael Relbe]

#9220 - Another approach, fully working

[Mikael Relbe]

The template file in t9220-mrelbe-r10140.diff​ is intentionally not indented as you probably want, Christian. As you can see, I changed to the whole lot here. The current indentation makes it easy to see what's going on, and easy to "correct".

[erne.castro@…]

Replying to cboos:

Not bad for a first patch ;-)

I made a few follow-up changes,

• the <a href="${href.wiki(...)}">... and last one ${dateinfo(last_date)} ago. line was a bit long
• the wording could be improved: instead of e.g. 5 revisions, first one 1 month ago and last one 1 day ago) I prefer to display: Removed all 5 revisions of the page, which was created 1 month ago and last modified 1 day ago. earlier in the page (this just a detail, of course, and you may disagree ;-) )
• the message was lacking i18n support (well, that link doesn't show much info for now, that will be hopefully be expanded one day)
• the Python code is OK, but after duplicating the loop, you could have continued and simplified the code a bit

As an example of how could things be polished further, I add a few more patches on top of yours. I hope this will help you to get started!

• Regarding the line to long… its ok… didnt notice it.
• The wording was not ok, I know… my mother tong is not english and some times simple phrases like this gets too complicated…. sorry.
• Yes… I didn't know how to do that… again… sorry.
• About the Python code… it way better how you rewrite it… thanks for the feedback…

Replying to mrelbe:

Argh… I was also producing a patch today… I'm supplying it anyway, perhaps it can create some inspiration: t9220-mrelbe-r10140.diff​

Sorry, I was searching the tickets and saw this one and it seemed easy enough for me to give it a try… and as it was with no news for 3 months… I took a chance.

If you prefer to you your patch is ok for, no hard feelings.

[Mikael Relbe]

Replying to erne.castro@…:

Replying to mrelbe:

Argh… /…/

Sorry, /…/ no hard feelings.

Don't excuse yourself! We are all glad that new people join in to help here, your initiative is very appreciated!

I am the sloppy guy who have let this ticket been sleeping for too long. Christian sent me a polite notice the other day, which led me to finally sit down today and do some job on this. (Disclaimer: I have recently switched working career, which totally drains my energy right now, and calls for 100% attention.)

But please pick up on Christians comments, and if you have the energy take a look at my patch as well, if you like, to get inspiration for functionality. If you want, we can work together with this, however I will be somewhat busy this week (but there's no hurry with this ticket). Note that I am also a newbie and have not yet earned/deserved the rights to make commits to the SVN repo. ;-)

[Christian Boos]

Replying to mrelbe:

The template file in t9220-mrelbe-r10140.diff​ is intentionally not indented as you probably want, Christian.

Actually, the <element><i18n:msg ...> on the same line is not my preferred way either, I just did this for keeping the same style that was already in use elsewhere.

The current indentation makes it easy to see what's going on, and easy to "correct".

Agreed, we can keep that, and more generally, I think I prefer this alternative approach ;-) I'll test it.

[Christian Boos]

Replying to mrelbe:

Replying to erne.castro@…:

Replying to mrelbe:

Argh… /…/

Sorry, /…/ no hard feelings.

Don't excuse yourself! We are all glad that new people join in to help here, your initiative is very appreciated!

Seconded.

I am the sloppy guy who have let this ticket been sleeping for too long.

Yep, sometimes it just goes that way in open source, no activity on a topic in months, then a flurry of patches ;-) Of course, Trac helps by seeing what's going on, but can't prevent people to start on their own at the same time. Any kind of "locking" scheme (like we had this "accept ⇒ assigned" state for signaling work in progress) is going to fail on the side of "stale" locks and could lead to block any activity.

But please pick up on Christians comments, and if you have the energy take a look at my patch as well, if you like, to get inspiration for functionality. If you want, we can work together with this, however I will be somewhat busy this week (but there's no hurry with this ticket).

And in the end, the whole point of the open source exercise is that often the collaborative outcome makes for a better result than what we could build alone.

That being said, there are a number of bitesized issues which are perfect candidates for your next patches ;-)

Note that I am also a newbie and have not yet earned/deserved the rights to make commits to the SVN repo. ;-)

Making your way to it ;-)

[Mikael Relbe]

#9220 - Replaces former patch. Final candidate (from me).

[Mikael Relbe]

The patch t9220-mrelbe-r10140-2.diff​ replaces the former patch and is my final take on this. It presents informative information for all of the use cases (delete single/multiple version and the whole page).

This patch takes Christians remarks into account regarding language style (which I liked). Tag indentation in the template file is now consistent (i.e. the header part, lines 17-31 in the new version, are identical to the header part in the former version except for re-indentation.)

However, I kept the loop, disregarding the simplicity inferred by the no-loop.patch​ since that simplicity will not enable the detailed info that is presented for all other use cases (deleting multiple versions in particular). But I'm pretty sure that Christian will find a way to simplify this anyway ;-)

[Christian Boos]

Thanks, looks great! I'll test and apply this evening (and no, the only simplification I can find is to do new_date = old_date = None, which isn't worth editing the patch ;-) ).

Keep an eye on #9222, which will probably need to adapt this "confirm deletion" page further.