Tags:
create new tag
view all tags

EcoTrashPluginDev Discussion: Page for developer collaboration, enhancement requests, patches and improved versions on EcoTrashPlugin contributed by the TWikiCommunity.
• Please let us know what you think of this extension.
• For support, check the existing questions, or ask a new support question in the Support web!
• Please report bugs below

EcoTrashPlugin Feedback and Development

-- Timothe Litt - 2014-01-22

Thank you Timothe for contributing this new plugin!

I created supporting topics (this dev topic, EcoTrashPluginAppraisal, Support.EcoTrashPlugin) and tagged the plugin topic.

-- Peter Thoeny - 2014-01-22

Timothe, I added the PackageForm to the EcoTrashPlugin topic, could you check the values?

-- Peter Thoeny - 2014-01-23

I'll provide feedback later. Just one thing now: How about checking this plugin into TWiki's SVN trunk? That way it is easy for us developers to try it out and help on doc fixes.

-- Peter Thoeny - 2014-01-23

Filed bugs on buildcontrib. Fixed package form to reflect what I tested - if you tested on the versions I removed, feel free to add the back.

I put the current code in svn, but didn't set 'developed in svn', as for now the master copy is in my private svn repo. I'll merge any doc fixes my repo until I can complete the migration.

I left Item7422 open in case there is an initial flurry of tweaks.

-- Timothe Litt - 2014-01-24

Thank you for checking it into SVN trunk! I installed it from trunk and poked around a bit. I like it a lot!

Some feedback and recommendations:

  • I recommend to move the TrashManager topic to the Trash web and call it WebTrashManager so that it is considered a system topic. Reasons:
    • Place system topics where the action is.
    • It is not good to have an access restricted system topic in the TWiki web - the WebChanges topics will prompt for login, e.g. it can't be indexed by search engines on the public site.

  • Remove access restriction on topic once placed in Trash web.
    • The Trash web is restricted to admin group by default.
    • If not member of admin group, the %TRASH variable could return a message such as "Only members of the XYZ group can use manage trash"

  • I sometimes move topics to the Trash at the shell level. Other admins do that too. This means, there is no META:TOPICMOVED. This results in funny looking table rows:
    • | o | TestUser1 | 1 | 24 Jan 2014 - 14:07 | ! Main.TWikiRegistrationAgent | |
    • Date deleted is wrong (don't show anything); deleted by is odd (show last change author or nothing).

  • Do not show full file path in the Trash Management Results page, not all admins need to know the system internals.
    • For topics show just path starting from data/Trash/
    • For attachments show just path starting from pub/Trash/

  • .lease files are not deleted

  • I recommend to just stick with the read-only mode of the skin. Why offer a toggle by clicking on the trash icon? FYI, there is a hidden edit link in read-only mode: The dash in the topic revision info line at the bottom is a raw edit link. That is sufficient for people maintaining this topic.

  • For now no issue, but for production release I recommend to change the META:TOPICINFO to "TWikiContributor" in all topics. The tools/fixtopicmeta comes in handy.

  • Instead of the twisty for instructions, how about adding a new "Instructions" tab? Saves space & reduces clutter.

  • Consider moving the "Irrevocably delete selected topics and attachments" button into the tab panes, e.g. allow topic deletion or attachment deletion, not both at the same time. Seems safer because you can look at the selection before you take action.

  • Maybe better to move the "Perform maintenance now" into a "Maintenance" tab. That way you can show the help in that tab pane, and the button below the help.

  • I recommend to hide the "Test mode" checkbox in production. Possibly add a configure flag to show or hide it.

  • What is "Unclaimed Files"? Needed?

  • I still think the "All" button is not descriptive enough and confusing when in the all checked state. I recommend to use multispan row and use descriptive label "Select all", toggling to "Clear all". Or maybe easier to show two descriptive buttons "Select all" "Clear all".

  • Possibly offer a "restore attachment" link for trashed attachments?

-- Peter Thoeny - 2014-01-24

One more feedback:

  • Not all Web* topics are system topics. Deleted topics of any web that starts with Web will have start with Web*. The question is how to identify system topics. It could be the topics found in the _default web, or possibly a configure setting.

-- Peter Thoeny - 2014-01-24

Two more:

  • Non-WikiWord topics are not linked or not linked properly, such as Non-Wiki_word (shown as unlinked Trash.Non-Wiki_word), WebNon-Wiki_word (first part shown as red-link).
  • It is not clear what the default sort order is. OS-X uses date modified for Trash, this looks like a good default.

-- Peter Thoeny - 2014-01-24

Thanks for the detailed comments. I'll see what I can do about them when I get a chance.

Couple of quick responses. %TRASH% does return an error for privileged functions.

Test mode does everything but delete; the install instructions strongly suggest that it be used at least at install time since misconfiguration can cause real grief.

As noted previously, the read-only skin does not make the topic read-only on my version of TWiki. On trunk, it misses several things under view (Raw,History,More Topic Actions).

Unclaimed files are those found in directories under pub/Trash that have no corresponding topic. The result of system failure - or folks who do things at the shell. They probably were attached to something at some point.

It is a standard UI paradigm to have a single unlabeled element, usually a checkbox, at the top or bottom of a column of checkboxes that toggles. Set all/clear all is the exception - which we shouldn't be using. If you hover over the button, it explains itself.

Sort order is date deleted (or if no metadata, file date). It's oldest first because (when you have max age), you can think of the expunge happening top to bottom. The sort is by a hidden prefix that's in ISO format. So it should be what a user expects. Is this not working for you?

I recommend to change the META:TOPICINFO to "TWikiContributor" in all topics. The =tools/fixtopicmeta comes in handy.= Please file this as a Build bug smile release should be doing that.

The protected topics list is an EXPERT configure setting. From Core.pm, the list and rationale:

# Web infrastructure topics - these aren't trash.  Note that ^ & $ are assumed
# to bracket the specified regex.  The don't because this regex is also used in
# a negative assertion.
# Users have no reason to change this; the cfg variable is to avoid a release
# if the infrastructure topic list changes unexpectedly.

my $protectedTopicsRe = $TWiki::cfg{Plugins}{EcoTrashPlugin}{ProtectedTopics}
  || q((?:(?:Web(?:Atom|Changes|Home||Index|LeftBar|Notify|Preferences|Rss|Search(?:Advanced)?|Statistics|TopicList|TopMenu))|(?:TrashAttachment)));

-- Timothe Litt - 2014-01-25

On Web* topics, for testing I renamed a topic WebSomething in Trash. It did not show up. I did not set the {ProtectedTopics} setting.

-- Peter Thoeny - 2014-01-25

It does for me. The restore link is wrong below due to the metadata - I copied a file for a quick test. I tried literally WebSomething and also WebTWikiRandomTopic47. I also used the GUI topic rename SandboxTestTopic0 to WebTrashSandboxTestTopic0; that also works.

Did you have a {MinimumAge} set? That prevents a topic from showing up unless it's been deleted that many days ago. (Policy case: you must wait for 1 backup cycle, tax records must be kept for 7 years, or other minimum retention rule.)

Or perhaps a protection error.

The Topic list is built with a regex %SEARCH% scope topic, ^(?!$protectedTopics$).*$ -- anything that would make that fail also would explain the topic not showing up.

If you can create a reproducer, or put yours on a machine that I can get to, I'll look at it.

Capture.PNG

-- Timothe Litt - 2014-01-25

New version in SVN addresses your list (and some other things). I'll wait to upload a new install kit until you've had a chance to give it a whirl.

Detailed responses:

  • I recommend to move the TrashManager topic to the Trash web and call it WebTrashManager so that it is considered a system topic.
Good idea. Done

  • Remove access restriction on topic once placed in Trash web.
Inadvertent. Done

  • If not member of admin group, the %TRASH variable could return a message such as "Only members of the XYZ group can use manage trash"
No action required, in existing code.

  • I sometimes move topics to the Trash at the shell level. Other admins do that too. This means, there is no META:TOPICMOVED.
Bad admin, bad. frown ~TWiki has a GUI for this. If we need a shell capability, write a script that uses the TWiki::Func API - like the attachutil that I did years ago. Not everyone has access to the shell. Those who do make mistakes. As you note, metadata is lost or made inconsistent. Not to mention what happens if a different Store backend is used.

Requiring people to use the shell makes the product hard to use. Allowing/encouraging people to do so causes trouble.

OK, so that's the theory, then there's also real life:

  • This results in funny looking table rows:
    • | o | TestUser1 | 1 | 24 Jan 2014 - 14:07 |![[Main.TWikiRegistrationAgent][]] | |
Fixed; if metadata doesn't return who did it, the last change user shows up.
    • Date deleted is wrong (don't show anything); deleted by is odd (show last change author or nothing).
Shows last modification date when no TOPICMOVED:date Shows last change author when no TOPICMOVED:by.

  • Do not show full file path in the Trash Management Results page, not all admins need to know the system internals.
Done - though there are risks.

  • .lease files are not deleted
Done. Good catch.

  • I recommend to just stick with the read-only mode of the skin.
See previous response.

  • I recommend to change the META:TOPICINFO to "TWikiContributor" in all topics.
See previous response. I hacked my build process for now, but BuildContrib should be responsible for this.

  • Instead of the twisty for instructions, how about adding a new "Instructions" tab? Saves space & reduces clutter.
Tried this; it doesn't work. Can't see instructions and data. Instructions tab distracts from tabs needing/offering action. Flow of page doesn't work. Twisty closed is not cluttered. Saves 1 line.

Not accepted.

  • Consider moving the "Irrevocably delete selected topics and attachments" button into the tab panes, e.g. allow topic deletion or attachment deletion, not both at the same time.
I didn't want to force someone to click three buttons. But I tried it and I think it's an improvement.

Done

  • Maybe better to move the "Perform maintenance now" into a "Maintenance" tab.
Done.

  • I recommend to hide the "Test mode" checkbox in production. Possibly add a configure flag to show or hide it.
See previous response.

  • What is "Unclaimed Files"? Needed?
See previous response. Latest code creates empty topics to hold them rather than delete; this allows admin to deal with the topic normally. Seems more conservative.

  • I still think the "All" button is not descriptive enough and confusing when in the all checked state...
See previous response. I've also changed 'All' to be a checkbox that indicates what action will be performed; title changes too. This may make you happier - if not, look at any webmail program.

  • Possibly offer a "restore attachment" link for trashed attachments
I was hoping you wouldn't notice the omission as it is non-trivial smile Done.

  • Not all Web* topics are system topics.
See previous response.

  • Non-WikiWord topics are not linked or not linked properly
Fixed

  • It is not clear what the default sort order is.
See previous response. (Ascending by deletion date)

-- Timothe Litt - 2014-01-27

Thanks for the detailed report. I updated from SVN. Feedback:

  • Manage trash panes are not visible unless the instructions twisty is opened. Cause: mode="span" instead of mode="div"
  • Good idea of adding conditional text in Trash.WebHome. Issues:
    • Escape [ and ] properly in MAKETEXT, see CPAN:Locale::Maketext - escapes ~[ and ~] (yes, really odd syntax)
    • The first paragraph has three backslashes.
    • The IF integer compare against 0 fails in case the plugin is disabled. Better to do string compare "'...'='0'"
  • Instead of the twisty for instructions, how about adding a new "Instructions" tab? Saves space & reduces clutter. -- PTh
    • Tried this; it doesn't work. Can't see instructions and data. Instructions tab distracts from tabs needing/offering action. Flow of page doesn't work. Twisty closed is not cluttered. Saves 1 line. -- Timothe
    • I do not think people expect to see instructions and trash table at the same time, human memory is long enough to toggle between trash topics/attachments tab and instruction tab. -- PTh

-- Peter Thoeny - 2014-01-27

Thanks for the additional testing. It's very helpful.

  • span vs. div seems to be a JQueryPlugin issue. The version I have on 4.2 works correctly, as did what I had on trunk. I updated trunk & now see the failure. Will change, but I think JQ span mode is broken.
  • Will fix the WebHome issues you noted.
  • I think we'll have to agree to disagree on the instructions. I tried your suggestion, but prefer what I have. I do like to see both at once. It's my code, so I'm sticking with my decision.

Will check these in later today.

Does this close all your issues?

One other thing - PackageForm doesn't exist outside twiki.org. So the metadata is not going in the master EcoTrashPlugin topic source. Shouldn't the form be in svn data/TWiki?

-- Timothe Litt - 2014-01-27

RE: MAKETEXT, yes I guess I should have escaped the brackets. But Current doc uses this example (and doesn't mention escaping):

%MAKETEXT{"Did you want to [_1[reset [_2]'s password]]?" args="%SYSTEMWEB%.ResetPassword,%WIKIUSERNAME%"}%

Further, TWiki's MAKETEXT auto-escapes brackets... Manually escaping them doesn't hurt, but I'm slightly confused. If escaping is required, the MAKETEXT variable doc needs to mention it. (It should mention the requirement to double tildes in any case) If not, why did you ask me to do it?

Rev 26956 has the changes - I also moved the WebHome text to a section in WebTrashManager - that helps keep the plugin independent of the core.

Please confirm that your issues are closed - I ran into some tricky quoting issues where older TWiki did things one way and trunk another. I think what I have now should work for everyone..

-- Timothe Litt - 2014-01-27

The plugin is coming along quite nicely! Good improvements overall. Yes, agree to disagree.

On TWISTY div or span mode, this is not a jQuery bug, it is an HTML spec question. As soon as you use block elements within the twisty you have to use div mode. This is an unwanted "gotcha" for many users, therefore we have the (not yet implemented) NewAutoModeForTwisty proposal.

I'll update the VarMAKETEXT example to reflect the proper (and odd) syntax.

The PackageForm only exists on twiki.org, and it should be that way. We need to have the capability to allow testers and non-programmers to update the form data without updating the whole package. It's data about the package not package data.

Additional feedback:

  • Now that the action buttons are within the tabs I recommend to use three separate forms, one for each action. Not sure if this is the case (not tested), it can be very surprising to select first some topics, then go to attachments to delete some attachments, and have topics deleted too. With this, the "test mode" checkbox is also better placed inside the tab.

  • I have not tested, but it looks like you show only topics within the date range specified in configure. I think this can be confusing (probably the reason why I did not see the manually renamed WebSomething topic earlier).
    • I think it is better to show all topics and attachments, regardless of date, and to color-code table rows based on date range (such as a pastel yellow to indicate old content, and pastel green to indicate new ones, and default colors for others. Ask me for jQuery code that does that.
    • You could add a toggle checkbox that hides/shows out of range topics using jQuery. Ask me for jQuery code that does that.

  • For original name, consider showing TestTopic1234 in Sandbox web instead of Sandbox.TestTopic1234.

  • Better to show TWiki standard ISO date for date deleted, such as "2013-03-02 - 02:58 " instead of "02 Mar 2013 - 02:58". The time could possibly be omitted, it helps avoids wrap around on smaller screen.

  • Instead of "Rehome topic" or "Restore topic" consider a more concise "Restore" - action is obvious, and helps avoids wrap around on smaller screen.

  • Tiny thing: There is a big gap between "instructions" toggle and the H2 heading, eating screen real estate.

  • (Future improvement:) Add a table row with filter input fields to narrow down on topics/attachments to delete.
    • Reason: The list can be quite long. Sometimes an admin is asked to delete topic FooBar for privacy/security reasons.
    • Implementation hint: See and copy code of ContactDbAddOn.

-- Peter Thoeny - 2014-01-28

So will the PackageForm data on twiki.org merge with the new topic when I upload the next version? Or do I have to re-attach the form every time I upload?

Since MAKETEXT auto-escapes brackets, do we really want to specify that people have to do it manually? I18n is already painful enough. Manually escaping what's automatically done seems like unnecessary pain... I'd vote for explaining that ~ needs to be doubled, and that brackets are auto-escaped...

The three tabs are already effectively three forms. The submit button only processes the items checked above it, and is labeled accordingly. The test box is global across all 4 tabs (Three item types + maintenance). That's why it's outside the tabs.

I only show what you can act on. The GUI shows topics at least as old as min age. The maintenance function expunges items older than max age. If you expand the "instructions" (above the tabs :-), you'll see a line like: All deleted items are shown. A minimum age policy can be defined in the configure script or Items deleted less than 30 days ago are not displayed due to your site policy.  This can be changed in the configure script.

To see everything in trash, there's already the web index. You might want to look at the %TRASH parameters in the web index and color code. (Note that in addition to color code, you have to add alt or title text to the rows as 10% of the (male) population is color blind, not to mention Braille and audio screen readers.) I guess I could eliminate the checkbox rather than limit the SEARCH.

The web.topic notation is used most everywhere in TWiki - and is shorter than 'topic in web'.

Date format - I used the same format that Store generates in the "topic moved, put it back" message. (In fact, I originally parsed it with CALC.) I have the ISO date internally (for sorting), but opted for consistency in display. Trash.TWikiRandomTestTopic48 moved from TWiki.RandomTestTopic48 on 17 Jan 2014 - 19:46 by TimotheLitt - put it back

Agree that 'Restore {Topic,Attachment}' is wordy. 'Restore' is the column heading. So Restore over a column of 'Restore's doesn't look right. Restore is different from Rehome. I'm not a graphics person - can you come up with a couple of icons? (Say, a trashcan with an up-and-out arrow and one with a trashcan pointing to a house? With titles for "Restore item" and "Rehome item", that might work.)

The gap between instructions and H2 was what I was trying to eliminate with mode=span frown (And it did work with the older JQuery!) I'll see if I can find a tweak - the H2 is generated by TML, and the Twisty by the Twisty plugin. It means injecting CSS.

Filter might be nice - for V2. I'd like to close this effort down. It was a quick and dirty solution to a decade+ problem. As I said initially, I don't want to make a career of infinite improvement. And Ctrl/F will instruct the web browser to find items for you! I do that a lot.

-- Timothe Litt - 2014-01-29

SVN rev 26960 addresses most of these.

  • The GUI now shows all items, but items newer than MinimumAge are not selectable.
  • The Instructions are now Instructions and Settings, and the policies are highlighted and re-worded to be clearer.
  • Date - I eliminated the time from the display (though with a big Trash web, this may turn out to be a mistake.)
  • Changed the Restore column to Action and the actions to buttons.
  • Closed the gap.
The others either were non-issues, or I decided to take no action.

Unless there is a bug, I'm inclined to upload the release kit and declare victory.

Let me know if you see any showstoppers.

Thanks for your observations; although I didn't take all the suggestions exactly as offered, they improved the design.

-- Timothe Litt - 2014-01-29

No need to take any actions on form when uploading a new version via BuildContrib, form values are retained.

On date, I really recommend using the standard TWiki date format. Since TWiki-4.2 (?), the date format is defined by the {DefaultDateFormat} configure setting with value '$year-$mo-$day'. An example moved message looks like this:

Trash.TestRename38 moved from Sandbox.TestRename38 on 2013-03-02 - 07:58 by JimmyNeutron - put it back

Thanks for all the contributions and for addressing some of my nagging^H^H^H^H^H^H^Hsuggestions!

-- Peter Thoeny - 2014-01-29

Your suggestions were constructive.

I initially thought about obeying {DefaultDateFormat} for I18n.

Oddly enough, It turns out that (even in trunk) there is no variable or spreadsheet function that returns it. The $FORMATTIME function doesn't have a $defaultdateformat keyword.

The only places that {DefaultDateFormat} is used are in REVINFO, SERVERTIME, DATE, DISPLAYTIME, and GMTIME. None of these take an date 'serial number' as an argument. So there is no way from TML to convert an arbitrary serial number to text using {DefaultDateFormat}, That's what's needed here.

For {DefaultDateFormat} to be truly useful, it should be available as a keyword in $FORMATTIME and as a MACRO in TML so it can be passed to other processes.

The only way to use {DefaultDateFormat} now is from Perl code. (This is a serious omission in the design, which I've noted before.)

For a general solution, SpreadSheetPlugin::Calc::_serial2date needs to look something like:

    my ( $theTime, $theStr, $isGmt ) = @_;

    my( $sec, $min, $hour, $day, $mon, $year, $wday, $yday ) = ( $isGmt ? gmtime( $theTime ) : localtime( $theTime ) );

    $theStr =~ s/(\$defdate)/$TWiki::cfg{DefaultDateFormat}/goi; # <==================== (But note that the cfg item uses different specifiers for 'month', so there's a conversion required)
    $theStr =~ s/\$isoweek\(([^\)]*)\)/_isoWeek( $1, $day, $mon, $year, $wday, $theTime )/geoi;

Along with something similar in TWiki::Time.

I'll leave that to you.

I can do some magic in %TRASH% now that it does most of the formatting work, but I shouldn't have to.

-- Timothe Litt - 2014-01-29

Rev 26961 Cleans up the Trash Manager topic when the plugin is disabled, and (with some ugly hacks) obeys the system {DefaultDateFormat} and {DisplayTimeValues} configuration parameters.

This has been uploaded to the Plugins website.

I'm declaring victory (and hoping that I didn't introduce any bugs.)

-- Timothe Litt - 2014-01-29

Thanks for the suggestion, it makes sense to enhance the SpreadSheetPlugin accordingly.

Since this plugin will be part of the standard plugins of the TWiki core we should take the TWiki SVN as the primary repository.

I'd like to do some doc fixes on the plugin topic. OK?

-- Peter Thoeny - 2014-01-29

Timothe, since this will be core plugin please be open to others to help maintain it.

-- Peter Thoeny - 2014-01-29

Now that the dust has cleared (I hope), no problem moving to the twiki repo. I expect, but don't take for granted, that the release meeting will agree to packaging it with the core.

Doc fixes are fine with me. I'm not working on it now. Please re-release when you're done.

While you're in the plugin topic, I noticed a small error in the install instructions. Under testing, it says:

If you have items in Trash but the presentation looks wrong, you probably have a old version of SpreadSheetPlugin

The link under SpreadSheetPlugin incorrectly goes to TablePlugin. Please fix this (I wasn't going to re-release for this.)

Happy to have help maintaining it. Changes in philosophy or enhancements - as usual I expect folks to consult so we stay on the same page and don't fork. Can be a quick e-mail, or the whole heavyweight feature request system. I prefer the former. I'm open to other ideas (as you saw), though I reserve the final vote as long as I'm the owner.

-- Timothe Litt - 2014-01-30

I'll update the docs.

I just run a report. Out of the 33 core extensions (plugins, contribs, skins) only one has a ContactAuthorFirst modification policy, all others have PleaseFeelFreeToModify. That one is the SpreadSheetPlugin owned by me. I only keep this modification policy because I am concerned about compatibility. So far I implemented all requests with code, and most without code. If you keep the EcoTrashPlugin as ContactAuthorFirst once a core extension I ask you to remain open to fixes and enhancements, even if you do not always 100% agree.

-- Peter Thoeny - 2014-01-30

I said I'm open. I've demonstrated that I'm open. I think all but one of your suggestions was adopted in some form.

When my name is on something, I expect the courtesy of consult before it changes materially. I don't withhold consent unreasonably.

I try to avoid unnecessary merges, so a contact ensures that the write token is available.

I also maintain software in multiple environments. While being culturally compatible with each is important, I don't want to see unnecessary divergence.

'ContactAuthorFirst' with me is not a closed door - just a request to coordinate. Don't make more of it than it is.

-- Timothe Litt - 2014-01-30

I have made the changes, please verify. I also added a screenshot.

-- Peter Thoeny - 2014-01-31

Thanks for all the time and effort. The doc changes look good, but I still need to check them on 4.2.3 to make sure that none of the optimizations that you used rely on newer features. One note: The reason for the continued lines in the topic (that you flattened) is that I edit in emacs & it's a lot easier to work with when the lines don't wrap.

I noticed that you added an extra )} in WebTrashManager - that was not needed. I think you missed the close brace/paren indexOf("Raw View")==0} <+++HERE+++> ).hide(); when you reformatted.

emacs is really good at verifying brace, bracket and paren matching. (Which works for TWiki variables as well as javascript. Thus, why I use it for this kind of work.)

Also, note that the .end().end() enables a .find() at the same level as the previous .find(), both of which are looking under the menutab. And the whole thing is an onLoad function. So in indented style, the code should look like:

$(function() {
  function none(chkboxes,divid,submitid) {
    if( !$("[name=\""+chkboxes+"\"]").length ) {
      $("#"+divid).replaceWith("<div class=\"ecoTrashStatusBox\">None found</div>");
      return 0;
    }
    if( !$("[name=\""+chkboxes+"\"]:checkbox").length ) {
      $('#'+submitid).hide();
    }
    return 1;
  }
  var some = none('delete_topic', 'topics', 'submitdeletetopics')
           | none('delete_attach', 'attachments', 'submitdeleteattach')
           | none('delete_unclaimed', 'unclaimed', 'submitdeleteunclaimed');
  $("div.patternTopicActions,div#twTopBarButtons").hide();
  $("div.twTopMenuTab").find("ul li a").filter(function() {
       return $(this).text().indexOf("Edit")==0
      }).hide().end().end()
                       .find("ul li ul li a").filter(function() {
      var t=$(this);
      return t.text().indexOf("History")==0
      || t.text().indexOf("More topic actions")==0
      || t.text().indexOf("Raw View")==0
    }).hide();
});
The extra }); that you added in rev 26973 was here.

On the related links - good idea. Should %TRASHWEB%.Webhome (the index) be included?

I'll install on 4.2.3 later and report back.

-- Timothe Litt - 2014-01-31

Did a once-over under 4.2.3.

The bug you introduced with the }); (which, at least with Firefox, breaks the initialization) needs to be fixed. You should be able to clip out the entire verbatim block above (from a raw mode view) and replace the entire contents of the SCRIPT tags with it.

Otherwise, looks OK.

Thanks again for all the polishing.

-- Timothe Litt - 2014-01-31

Thanks for catching the bug, Timothe! Funny thing is, the code worked on my FF on Mac. I fixed it, and I did an additional doc fix. In SVN trunk, 6.0 branch, and also released.

-- Peter Thoeny - 2014-01-31

Small GUI issues I noticed:

  • The action button text ("Irrevocably...") is too small, should be same size like normal text
  • The "Restore" button text is bold, looks better if normal

-- Peter Thoeny - 2014-01-31

On windoze, it failed the whole script block. Don't have a mac - but I suspect the JS console would have shown an error. In any case, brace matchers are a good thing.

On the buttons, I don't control those, the skin does:

  • The action button text is a submit type button with CSS class twikiSubmit The size is the same as any other submit button - but the icon probably makes it look smaller, as does the long label.
  • The Restore/rehome is a link <a> with CSS class twikiButton
Neither has any style overrides. F12 tools will show the inheritance.

I'd rather not customize those, as it would be different for each skin (or wrong for some). If they're wrong here, they're wrong elsewhere, so the skin should be changed, not the plugin.

I'm really cautious about functions that can do massive deletes, which is why the icon and the long label are used. If someone deletes a bunch of stuff by mistake, they can't claim they weren't warned...

-- Timothe Litt - 2014-01-31

Agreed, better to fix UI issues at the core.

-- Peter Thoeny - 2014-01-31

For reference, here is the issue using various tags:

  • - using button tag
  • - using input type submit tag

  • Restore - using a tag
  • - using input type submit tag

-- Peter Thoeny - 2014-02-04

Both cases are using the current tags for cause:

  • Input type submit won't allow an image in the button.
  • Restore/Rehome don't submit a form, they're links to the attach pages.

Both are valid applications of the tags/classes.

If this is judged important enough to invest time in, it should be fixed in the pattern and/or core CSS.

Note that the 16 x 16 icon is making the tag about 5px taller, but the text remains the same size. So you may want to set a max-height on images in button tags. Or scale the text to a percentage of the button height.

For the a tag with class twikiButton, looks like increasing the padding would make the sizes match. Bolding of the label is coming from style.css:186 (a.twikiButton & a bunch of others) - which applies it to both the a and the input versions. a with twikiButton is intended to look like an action button, even if it does a GET rather than a post. Here, we are initiating an action.

Actually, I see the button text as clearer with the icon; the input/submit version looks excessively bold, and not as easy to read. but that could be just me.

In any case, EcoTrashPlugin is not at fault. It might be better to open an item against Pattern and/or core CSS to work this.

While some tweaks may improve the presentation, I don't believe they are mandatory - certainly using the plugin with released TWiki doesn't depend on them.

-- Timothe Litt - 2014-02-04

I filed TWikibug:Item7427 to fix the PatternSkin issue.

I plan to fix the buttons on the EcoTrashPlugin for sites that have an unpatched PatternSkin.

-- Peter Thoeny - 2014-02-04

The EcoTrashPlugin is now updated using TWikibug:Item7427, in SVN trunk, 6.0 branch, and package uploaded.

-- Peter Thoeny - 2014-02-04

Thanks for fixing PatternSkin.

I don't like changing the CSS in the plugin, as it assumes PatternSkin and the change will never be removed from the Plugin. Not all sites use PatternSkin, and the skin may change in the future. Such a change should not require updating the plugin.

A better approach to helping sites with unpatched PatternSkin would be to distribute a patch to PatternSkin with a note in the install instructions. This would keep the plugin independent of PattternSkin. Such a patch could even be auto-installed in a post-install script. This is consistent with, for example, the way TopMenuSkin and GenPDFPlugin's instructions tell installers how to modify templates.

-- Timothe Litt - 2014-02-04

DontMakeMeThink: I put the CSS into the plugin so that users have a good first time experience and are not bothered with extra steps. The CSS does not hurt if another skin is used or if the PatternSkin CSS changes in an in incompatible way (which is unlikely). You or I can remove the CSS from the plugin topic if you feel strongly about the CSS change. Since this plugin is scheduled to be included in the core I would like to see flexibility in having it maintained by the community under your guidance.

-- Peter Thoeny - 2014-02-04

I agree with the goal of a good first-time experience. I appreciate the work you've put into finding the skin issues. I'm happy to have others contribute to maintaining it.

The issue is how best to distribute the fixes, not whether they're a good thing.

The packaging that you picked is expedient, but breaks modularity. I've been a product architect and a serviceability architect (among other things); these things matter.

I would strongly prefer another approach. Some choices:

  • The installer could look at the TWiki (or PatternSkin) version and apply a patch when needed. This can be a patch file, or a 1 statement perl script. This would still give the good first-time experience, without having the plugin override the skin. It also fixes other uses of the same constructs as they inherit from the skin. As a fall-back for those who don't use the installer, the manual install instructions can include patching. When the new skin is released, it will over-write the patch.
  • We can move the CSS into a separate file, and have 2 versions - one with the work-around. Teach installer (or tell people) to rename for TWiki pre-Kampala. This has the drawback that it won't be automatically undone when the skin is fixed. But it does eliminate the need to patch.
  • We can put the entire pattern skin file that you modified into the kit, and install iff it's newer than the one on the system. That might (or might not) have issues with older TWiki versions.

I suggested (1) because it distributes the minimum amount of PatternSkin, it goes away by itself when no longer needed, is easy to qualify (test) and it still is automatic

(2) may be easier on, for example, windows where you can't count on the unix patch utility. But it doesn't go away.

(3) avoids patching, but risks distributing more of PatternSkin than needed - possibly creating issues.

Any of these provide the good experience without putting the code in the wrong place.

The problems with putting the work-around into the plugin include: It will be forgotten, not removed when the updated skin is released. It doesn't fix the same issues for other users - I know I'm not the only user of links styled as twikiButtons. (I may be the first to include an image in a submit button.) And over time, other 'fixes' will get added in the wrong place.

Doing it your way won't cause the world to end. But I don't think it's the best approach.

-- Timothe Litt - 2014-02-07

I do not have a strong opinion which way to go. Things to consider when you rework this:

  • Easy of installation
  • Most admins use configure to install extensions, install script is not so much used

-- Peter Thoeny - 2014-02-08

Edit | Attach | Watch | Print version | History: r35 < r34 < r33 < r32 < r31 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r35 - 2014-02-08 - PeterThoeny
 
  • Learn about TWiki  
  • Download TWiki
This site is powered by the TWiki collaboration platform Powered by Perl Hosted by OICcam.com Ideas, requests, problems regarding TWiki? Send feedback. Ask community in the support forum.
Copyright © 1999-2024 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.