Posted by Zen on March 8, 2006 at 8:17am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | locale.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | zroger |
| Status: | closed (fixed) |
| Issue tags: | i18n sprint, Screenshot, Usability |
Issue Summary
The search strings page should either use sessions or GET to retain search queries across pages. Currently, there's no way to return to the results page after an operation (delete, edit..) has been performed.
-K
Comments
#1
Maybe it should be re-implemented like user_search?
#2
Yep.. something like: search/en/translated/request+new+password
The current setup actually scans $_REQUEST as opposed to just $_POST. So I believe that something like this was possibly already intended..
-K
#3
I doubt it was intended, the locale search code is quite ancient. It would be great to replace it.
#4
+1 for this... it's really tedious to keep typing in the search string again and again.
#5
Changing title. The brainstorming here (http://groups.drupal.org/node/3916) resulted in some ideas about what should we do. The current state of the art interface might be the watchdog filter. A task list:
- remember search form settings
- provide a default list of strings to edit before even trying to search
- maybe provide a mass editing interface focusing on one language (submitting tens of translations for one language at once) and the current interface to be able to translate one string to multiple languages at once
- let the user walk to the next/previous editing interface with both approaches
#6
Sorry if this is too off topic (does not target the issues listed above). But I've added a simple feature to the search interface, that made it much more useful to me: The ability to search by (restrict to) certatin "locations".
A location is the thing that (in my theme) is displayed in a smaller font below each phrase in the result list, and it tells on what URL the translatable phrase was first found. The patch is made against v. 5.5, using diff -u, but I do not have a CVS checkout and the first lines that tells the filename is perhaps not in accordance with standards.
I'm developing a module of my own, and thought it would be nice to list all its (relatively few) phrases, and translate them using the web interface. But since there are some thousands of untranslataed phrases from other modules (many administrative phrases) I got a very long list (89 pages) when performing a search for all untranslated strings. But using this patch I can filter away those unrelated phrases.
The issue about search being forgotten if you click on an 'edit'-link remains, but I handle it by opening all edit-pages in background tabs (so the search window remains). That is of course not the ideal solution.
#7
Location could be a few things unfortunately. For translations imported from an existing .po file, the location will be the source file name and line number from where the string was extracted, not any URL. Also, strings might appear on multiple URLs. Eg. on admin/content/node and node/43 This makes the location field a bad value to base search features off.
#8
OK, I didn't knot that about import of .po files.
But I did realize that in the case when the location is a (relative) URL, it seems to be the URL where the phrases was first displayed. So if a module is always (or at least before translation) used in such a way that the URLs contain something particular (e.g. the module name) then all the used phrases can be found by a location search. For instance, a search for "mymodule" would find "mymodule/412" as well as "admin/mymodule/foo".
But I guess what you wrote makes the usefulness of location search limited to this very case.
#9
#10
As far as I know, at least "remember search form settings" is already implemented.
#11
#12
I am starting work on a patch to re-work the existing interface to be modeled after the dblog interface, as part of the i18n sprint
This should be considered step 1 of cleaning up this interface, with other features to follow.
#13
Here's a first go at a patch. The main interface screen has been changed to resemble the dblog interface, with filters inside of a collapsible fieldset at the top, with the table below. A default list shows even before filters are applied.
Destination query strings have been added to the edit and delete links to facilitate repeated entries.
The text in the tab has been changed to Translate, from Search, and the path has likewise changed to admin/build/translate/translate.
The interface beyond this has not been touched.
#14
oops. left a debugging message in there.
#15
The last submitted patch failed testing.
#16
New patch, with modified tests. Passing locally for me and stella.
Also removed the #collapsed logic, agreed upon in IRC.
#17
This is a major improvement for the usability of the string translation interface. I've tested it and it works very well.
To summarise the changes:
(Testing bot is marking patches as "needs work" due to failing Field tests committed today.)
Cheers,
Stella
#18
Tested too, works as expected.
#19
Found 6 additional places that reference the old path of admin/build/translate/search.
Updated patch replaces those. stellas summary in #17 above still applies, with this one addition.
#20
Reviewed #19 and works as expected.
#21
Nice work on this patch! I really appreciate stella's summary @#17.
First off, can you please post before/after screenshots of the interface (same for any other UI-affecting patches)? I know that these are just minor changes for now, but it would really help get broader input from folks who don't have patching-fu but will be affected by this interface change.
Now, for the annoying webchick pass. ;)
+ // Add CSSNeeds a period at the end.
+ //'where' => "??",Er. Huh? :)
+ '#theme' => 'dblog_filters',I wonder if it makes sense to name this something different and pull it out to system.module or similar. Otherwise, it seems to me that this code is going to fail spectacularly if people have syslog rather than dblog installed. Then as a separate issue we should maybe create an API for "filters to show above admin pages" and merge all of these types of forms (node admin, user admin, etc.) to use the same system.
+function locale_translation_filter_form_submit($form, &$form_state) {...
+ return 'admin/build/translate/translate';
+}
I believe what you want there is
$form_state['redirect'] = 'admin/build/translate/translate';The fact that all tests were passing and there were 6 missing references tells me that our locale system tests must have huge holes in them. As a separate issue, could you itemize what these holes are and make a list of the tests we still need (and hopefully write them as part of the i18n sprint? :D). Ideally, our testing framework should SCREAM IN HORRIBLE, UNBEARABLE PAIN when #16 is applied. ;)
In
_locale_translate_seek()there is a many-thousands-line-long dynamic SQL query building trainwreck of code. ;) I realize you're just re-working what was there before, but this needs to be migrated to DBTNG. See documentation for dynamic queries at http://drupal.org/node/310075. There are quite a few people in #drupal with expertise here, so don't hesitate to ask for help if you get stuck.That should be enough to keep you folks busy tomorrow. :) Thanks for all of your hard work!
#22
subscribing.
#23
Posting screenshots.
#24
About the SQL Query - whole locale system needs to be rewamped to DBTNG, I have created an issue for that #369024: Revamp locale module to DBTNG to separate efforts.
#25
These screenshots are much better, but with or without the query conversion it still needs the rest of webchick's comments applied to the patch, so back to CNW unfortunately.
#26
Here are the 6 places.
locale.inc
1. locale_translate_edit_form(). drupal_goto() if the source translation cannot be found.
2. locale_translate_edit_form_submit(). Used for the $form_state['redirect'].
3. locale_translate_delete_form(). Passed as the $path argument to confirm_form() for the cancel link.
4. locale_translate_delete_form_submit(). Used for the $form_state['redirect'].
locale.module
5. locale_help(). case 'admin/settings/language', line 35
6. locale_help(). case 'admin/build/translate/search', line 58
Any suggestions for tests are welcome.
#27
new patch addresses webchick's first 4 concerns in #21.
No new tests, no DBTNG.
#28
Retag (cross posts #25 & #26)
#29
I think this can be marked RTBC as all but two of webchick's concerns have been addressed. Those concerns are now being dealt with in separate issues:
I've retested the last patch, reran the simple tests, ran it through coder again and I think it's good to go.
Cheers,
Stella
#30
Wow, now that I've actually tested out the patch rather than just read it, I can't over-state how incredibly this improves upon the existing interface in basically every single possible way. Here are some screenshots to explain:
Before Patch
After Patch
I actually don't even know how people are using this currently without ripping their hair out in bloody clumps.
It is therefore with great pleasure that I say, COMMITTED TO HEAD. :) Thanks SO much!
We probably can't back-port this entire thing to 6.x because the interface change is quite dramatic, but it would be nice to at least get rid of the "WTF" form redirection on the string save form, and have it remember the last string you typed in.
#31
#32
Impressive UI.
But the filter feature looks broken. It only find 9 strings ready for translation.
For example I added a new French language and proceeded to translate date string. When looking for Sunday it did not find anything. Looking at all unstranslated string, it found out only 9. When translating one of these strings, the French Built-in interface showed only: 1/9 (11.11%).
Shall we open a new issue?
#33
I don't see any reason to backport this to Drupal 6. I've used this interface a lot of time without losing my head.
However, for D7, what an improvement!!!!
#34
@xmacinfo: Yes, I noticed that bug before this patch was applied. I think it *might be #338630: Locale is unable to rebuild lost Javascript translation files, but I'm not positive.
#35
Please try actually setting French as default language and browsing site. Check if the strings are in the UI now. We all made this mistake when testing this patch. The strings are saved into database only when you language is not English...
#36
As suggested, I set French as default. As you mention, now I have 5 pages of strings ready to translate instead of a single page.
However, I tried to translate months names, to no avail. And looking to the Translate interface Overview screen, French have now: 1/202 (0.5%). The first value is not relevant, it's the actual string I did translate, but the 202 is way too small for a D7 install.
As Webchick suggests, the issue might lie somewhere else.
#37
Yes, please start a fresh issue for Locale module oddness; this issue is only about the UI, and it is fixed.
#38
If someone wants to translate anything in D6, he/she will use the l10n_client module or import the translated .po file. Hopefully this change will make a good addition to the existing (usable) tools.
#39
Automatically closed -- issue fixed for 2 weeks with no activity.