|
Poznámky k chybe |
|
|
|
also i would add something like
['up']['add'] = array('cancel');
['down']['add'] array('save');
which would override defaults and would display cancel button at the top and save button and the bottom of the page (you want users to first to read and only then to add), maybe regardless of navigation in $opts. so you can have up and down navigation in LF and only down in VCPDA. sometimes it's usefull. |
|
|
|
|
Interesting ideas.
Do you agree to postpone them after 5.4 will out? |
|
|
|
|
ok,
just to finish at least the first step, does
$opts['display']['buttons'][$mode]['up'] and
$opts['display']['buttons'][$mode]['down']
look reasonable ? $mode is either add, view, ...
if is_array then this array is used to display buttons, else classic way is used (search for U/D in navigation option and display all possible buttons). |
|
|
|
hbernard
|
|
2004-01-06 09:21
|
|
Some thoughts :
1. What is the referential of button's 'names' ?
Shouldn't create an array which describe with completness the button (value, name, type, optionnal js, css class, etc... ? Then the keys for this array will be the referential
Something like
$this->buttons_reference = Array(
'saveadd' => Array('Save', 'operation', 'save', true, ),
...
);
2. About the up/down stuff, why not simply make this work :
$opts['display']['buttons']['add|U'] = Array();
$opts['display']['buttons']['add|D'] = (another)Array();
Too much parsing ? Yees, maybe a bad idea from me
3. I would like to see default layout defined internally with $opts['display']['buttons'], to make the code more readable (and free of too much tests against $this->display['buttons][...])
But then we have to merge internal layout with optionnal custom layout.
However, I think this point is important to clean up internal buttons handling. |
|
|
|
|
1. buttons_reference[operation][button] = Array( what needed to call htmlSubmit) will not be that hard to implement in core class, there are only several types of buttons (concerning name, label/value, css):
- cancel button: cancel (name: cancel, value: $value, no js, no disabled)
- confirmation buttons: save, more (name: $operation.$value, value: $value (except for delete and apply), js validation, no disabled)
- goto operation buttons: add, view, change, delete, copy (name: operation, value: $value, no js, no disabled)
- navigation buttons: prev, next, >, .. (name: navoperation, value: $value, no js, sometimes disabled)
- dropdown box (a very special case)
- navigation within non LF mode: similar to navigation within LF but preserving operation and reducing inc to 1 (will that work ??)(just an idea)
css is always ($value, $position) (except for <<, < and >, >> )
the only really bad exception is with delete (in LF) vs. really_delete (in delete mode). maybe introduce a new label ('Delete!') ?
so grouping of these buttons can be done merely by name/label (and mode, due to delete issue).
2. to stay consistent with rest of configuration, we should use $opts['display']['buttons']['up|AC'] = Array(...); that looks wiered ;)
3. one function to return array of buttons (either user defined or defaults), display by one foreach(that array) calling to another function sorting names to groups as shown earlier and displaying them. that should be it. |
|
|
|
|
a diff against v1.115 uploaded. patch(1) complains but result is working.
changes:
new display_buttons() function which returns all buttons using display_button() function
some changes in the constructor, list_table (make num_records global) and obvios old display buttons
features:
$opts['display']['buttons']['view']['up'] = array ('change','cancel');
tells which buttons to be displayed
$opts['display']['disabled'] = 1; 1 is current state, 0: don't display disabled (prev, next)
enjoy and comment |
|
|
|
|
since i'm lost in my patches, i've posted a bad patch (just copy and paste in place of another code).
it's just a code cleanup, minor new variables were included (number of pages, ...) so that this handles all text at top and bottom of pme page.
and please comment ;) |
|
|
|
|
uploaded a new patch against current cvs.
buttons are declared as
$opts['display']['buttons']['A']['up'] = array ('cancel');
keywords (save, more, cancel, add, change, view, copy, prev, next, drop_down, goto, first, last, >, <, >>, <<) are translated into apropriate buttons.
custom button can be created by substituting keyword with array ( 'name' => 'name', 'value' => 'value', 'css' => 'css_class', 'disabled' => false).
or simply array ('code' => '<input ...>')
'next' shows disabled button when appropriate, while '-next' doesn't shows button if disabled. '+next' always shows button. |
|
|
|
|
Here is a proposal we made yesterday with michal:
Goals:
------
1. Custom button order.
2. Possibility to deny particular button.
3. Buttons for custom actions.
API
---
$opts['buttons'][$page_type]['up'] = array(...)
$opts['buttons'][$page_type]['down'] = array(...)
$page_type is single letter from this set: ACPVDFL
Implementation details:
-----------------------
1. How custom fields are created?
There are keyword strings (means system buttons) and array (means custom fields).
array('save', 'more', 'cancel', // these are keywords
array( // array means custom field
'name' => 'some_name',
'value' => 'Some value',
'css' => 'same_as_name',
'js_validation' => true/false
-- or simply --
'code' => '<select...><option...></select>'
)
)
Note: when code is used, all other elements are ignored.
2. How can I choose between disabling-next/prev and removing-next/prev?
There is a possibility to prepend + or - before button keyword:
+name -- always shown
name -- disabled when not appropriate
-name -- not shown when not appropriate |
|
|
|
hbernard
|
|
2004-04-09 20:09
|
|
I agree with this kind of API.
But beware to keywords definition.
I mean, save in Add is not the same as save in Copy or in Change.
IMHO, we have to use cgi buttons names exact keywords. Even if buttons have same role in different form (page_type), they should have different name.
I know API make a distinction by using separate arrays, but I think this will avoid some further API change incompatibility.
Just my 0.02 € |
|
|
|
|
due to no js validation in delete, new version uploaded.
save button is defined as saveadd/savechange/savedelete. even though it is displayed as Add/Save/... |
|
|
|
|
Michal, there were several problems with your patch.
After hacking a bit, I made another one.
Please try it and report, if it is OK and can go to CVS. |
|
|
|
hbernard
|
|
2004-04-24 15:07
|
|
If I may give my opinion on this :
- this is very nice patch :-))
But, please, don't you think it would be better if buttons were defined in an unique namespace.
I mean, 'save' is not unique. It's just the mode which will make the difference.
I feel strongly this is a bad idea to multiple cancel/save name.
In last weeks, we had to do this on the form level, and maybe there are good reasons to think that this is a good policy to apply everywhere. |
|
|
|
|
|
nepto: patch looks working, afaik only several variable names and declaration were changed... |
|
|
|
|
Actually there was several misfunctionalities, for example "Change" button does not work on "View" page.
However, should I process with commit?
Last hbernard comment looks relevant and important... |
|
|
|
|
'change' on V page is the same as in L or F page, so it should work....
about unique namespace. currently, all variable names are defined in unique way (depending on page type and button name/value). if in a mode, there cannot be more applicable 'save' buttons than just that one (the correct one). in what way should it be unique ? |
|
|
|
hbernard
|
|
2004-04-29 13:35
|
|
|
what if I want to have direct savedelete button in change mode ? |
|
|
|
|
Nice catch, this *should* be possible.
So my patch has to be altered... |
|
|
|
|
with savedelete, there are two 'delete' buttons: savedelete and operation_delete, both with the same definition in $this->labels. i definitely wouldn't want users to mix these two...
maybe extra js warning with savedelete ?
so we could make save_delete and operation_delete syntax in buttons.
and if all fails, you can use advanced syntax (array ( 'name' => ...)). where you can describe each button in all complexity.... |
|
|
|
hbernard
|
|
2004-05-03 16:17
|
|
1. about unicity
Well, looking at others bug (language unsepecific operation and cgi prefixes), the policy maybe should be :
Planned unique identifier are :
delete in list mode >>
- name : "PME::sys::operation"
- untranslated value : "PME::op::Delete"
- translated value : "Delete"
delete in delete mode >>
- name : "PME::sys::savedelete"
- translated value : "Delete"
- (not in bug tracker) untranslated value : "PME::subop::Delete"
So I would suggest (untranslated):
operation_Delete
savedelete_Delete
Yes, this seems silly to have savedelete_Delete (two times delete) but I think this make sense.
API have to be done in the same way everywhere.
2. what do you mean with extra js warning ? (something like "are you sure : yes/no ?")
I dislike this, because that is already the role of delete form...
3. About users confused by ability of having two different "Delete" buttons.
yeah, I know my catch is something in the corner, but i really think that technicals users will want to do things like that.
on the other hand, using custom
array("name" => "savedelete", "value"=>"Delete", "css"=>"save", "js_validation"=>false)
will add a corect savedelete button, so maybe you're right to want only "regular" buttons (save, more, cancel).
In fact, what I dislike with non unique naming scheme (save, more..) is that we are mixing contextual name (save, more..;) and global names (change, add, etc...).
I feel strongly that this is confusing in another way, and that's why I suggest (1). But again, there are workaround if you choose to go the way the current patch is. |
|
|
|
|
i believe that 95% of user just want to remove several buttons from current state and 4% want to add very custom button (like dropdown to select a variable then used in 'filters'). that's why i did this in such a simple manner....
1. i thing delete is the only case for save* in other than * mode. can you imagine another catch like this ? (you cannot have savechange in V, or moreadd in L :-) )
and other save* buttons need a form, so they will not be a standard link... but i agree, this should be implemented, just to have all clean.
2. yes that's what i meant. just for savedelete in V mode... since a user cannot tell the difference. maybe extra option 'js' in array (name => ...) which would put 'js' code within the tags (i know, OT, so later)
3. about long vs short names: i preffer one with less characters :) question is what will produce less forum questions :), so i preffer one with less forum questions... |
|
|
|
hbernard
|
|
2004-05-05 08:25
|
|
1. yes, delete is the only case I can imagine _within_ PME.
But if you use sub form, or external sources, it have to be possible to add directly records from external links.
2. technicals users have to take care to not display savedelete_delete and operation_delete buttons in the same forms. This is not usable. OT => OT ;-)
3. my proposition :
<not_prefixed_button_cgi_name>_<not_prefixed_nor_translated_button_cgi_value>
then :
operation_Delete and savedelete_Delete (exact matching case).
But, to be easier for some users, it is possible to imagine default aliases (which would be replaced at run time)
Delete and savedelete or whatever you think the best is.
95% of users will just pick the good names in the documentation. |
|
|
|
|
a new file uploaded, there are just three changes against nepto's version:
- if $opts['buttons']['V']['up'] is set, those buttons are displayed, regardless of $opts['navigation'].
- user can specify $opts['buttons']['V']['up'] = false; to not display any buttons (again regardless of $opts['navigation']).
- if $opts['buttons']['V']['up'] is not set and $opts['buttons']['V']['down'] is set, U navigation is pme default (as set in $opts['navigation']) and D is custom.
this allows to have U navigation in V, D in AC and UD in FL. |
|
|
|
|
1. concerning to "savedelete"; thinking about this, this is only the button, which can be used on other pages (copy, change, view) other that its native one (delete). Thus we can add simple "savedelete" into list, which can be put on change page which functionality will be immediate record delete. Than you can have "delete" and "savedelete" on the Change page, both doing different things but with the same label :-)))
2. Michal, nice changes, I was though about changing $opts['buttons'] to $opts['display']['buttons'] - the main reason why I firstly disagree with $opts['display']['buttons'] was, that this is collection of boolean only options. We can make $opts['display']['buttons'] as boolean (false will turn off all butons on the page, true will be default), where array can be defined as an "advanced true" value, with buttons configuration.
So $opts['buttons'] or $opts['display']['buttons']? :) |
|
|
|
hbernard
|
|
2004-05-11 06:21
|
|
1. So this means, when in "Delete" mode, save and savedelete will have same meaning ?....
I agree this is a simpler API.
You and michal seems to really want to go this way, so OK. ;-|
2. Can we stick with $opts['buttons'] ? Your agument about array of boolean seems to be a very good one to me.
Extended features should desserve their own section in opts, if not field defined.
3. About michal changes :
This means 'buttons' have precedence on 'navigation' ?
This is just a matter of taste, of course. But this seems odd to me, but don't worry about my opinion here, I will like the way you both like. |
|
|
|
|
1. ok
2. $opts['buttons'] is fine.
$opts['display']['buttons'] as boolean: i'm not sure this will add a lot of functionality. either it will turn off all buttons (not sure that's wanted), or some advanced array like configuration which would be a duplication of $opts['buttons']. or make 'default' which is equivalent to commenting out part of configuration code.
3. afaik $opts['navigation|AC'] won't work as expected. so 'buttons' is the only way to achieve different navigation on different pages. |
|
|
|
hbernard
|
|
2004-05-11 22:48
|
|
2. it seems michal and I agree on this.
3. only rule I would change is
- if $opts['buttons']['V']['up'] is set, those buttons are displayed, regardless of $opts['navigation'].
to:
- if $opts['buttons']['V']['up'] is set, those buttons are displayed, if 'U' is set in $opts['navigation']. ('navigation' has precedence on 'buttons')
If you want to have different navigation on different pages, you'll have to explicitely write :
$opts['buttons'][...]['up'] = false;
or
$opts['buttons'][...]['down'] = false;
...
AND allowing displaying Up and Down buttons with :
$opts['navigation'] = 'UD...';
Yes, this is more verbose but much clear (to me, at least ;-)
Said differently rules will be :
- navigation has precedence on buttons :
navigation is responsible of _allowing_ (or not) Up or Down buttons.
- if $opts['buttons'][...]['up'] is not set, then it take its default buttons
- if $opts['buttons'][...]['up'] is set to an array, then, these will be the buttons ;-)
- if $opts['buttons'][...]['up'] is set to false, then, this is like a _void_ array (Although allowed by navigation, there will be no buttons displayed. |
|
|
|
|
3. so $opts['navigation'] would be used not to display buttons but to hide some buttons.
and implementation:
check if navigation allowed
check if $opts['buttons'] is set (and return if false)
else apply standard buttons
that's fine with me (just must make 'UD' navigation as default)
4. documentation should be in chapter 3.6 Navigation ? |
|
|
|
|
new version (4) uploaded. it summarises all that was to change.
changes from version 3:
+ savedelete
+ precedense of 'navigation' over 'buttons' (if 'navigation' does not allow D buttons, they are not displayed even if 'buttons' specify it.
currently, i'm working od documentation. |
|
|
|
hbernard
|
|
2004-05-16 19:36
|
|
Michal, I didn't test, but I have a few comments on your last patch :
A. About function display_buttons($position)
.../...
+ foreach (array_keys($this->buttons) as $key) {
+ if (stristr($key, $this->page_type) && isset($this->buttons[$key][$position])) {
+ $buttons = $this->buttons[$key][$position];
+ if($buttons == false) return;
+ is_array($buttons) || $buttons = array($buttons);
+ break;
+ }
+ }
A.1. I was wondering why you were looping on the whole array, and I found that this code allow to have same definition for multiple page_type !
so $opts['buttons']['LF']['up'] = Array(...) would be correct.
This seems to me an undocumented and undiscussed feature ;-)
Did I understand it well ?
If not, why simply not test :
isset($this->buttons[$this->page_type][$position]) ? (without looping ...)
A.2.
+ if($buttons == false) return;
should be
+ if($buttons === false) return;
just to be sure of the API (explicit false boolean)
A.3.
+ if(!is_array($buttons))
+ $buttons = $this->default_buttons[$this->page_type];
should be the else clause of the first if statement :
+else $buttons = $this->default_buttons[$this->page_type];
B. just to conform PME code convention :
+ $position == 'down' && print('<hr class="'.$this->getCSSclass('hr', 'down').'">'."\n");
and
+ $position == 'up' && print('<hr class="'.$this->getCSSclass('hr', 'up').'">'."\n");
are floating around and should use echo in place of print ;-)
C. Good work !
Except comments above, seems ok to commit.
chapter 3.6 seems a good place for incoming documentation. |
|
|
|
hbernard
|
|
2004-05-16 21:40
|
|
I did some tests.
Patch works fine. :-)
My comments above are still valid.
And I found these (bad) side effects :
1. This is cause duplicate <div ...> output and should be removed :
+ if ($this->tabs_enabled()) echo '<div id="phpMyEdit_tab0">',"\n";
(there is another (correct) place where <div id="phpMyEdit_tab0"> is outputted)
2. + $total_pages = max(1, ceil($total_recs / abs($this->inc)));
is wrong, it should be :
$total_pages = max(1, ceil($this->total_recs / abs($this->inc))); |
|
|
|
|
A1: yes, it allows ['LF'] declaration :)
however, ['LF']['up'] with ['L']['down'] might behave strangely... (same as *|LF with *|L with field options) so i'm not sure whether to implement and/or document.
A2: ok
A3: no, with ['L']['up'] = array (..) and ['L']['down'] not set (so default should be outputed).
B: ok
C: thanks ...
1. ok
2. what is the difference ? :) |
|
|
|
hbernard
|
|
2004-05-17 11:19
|
|
A3 : OK
2.$total_recs seems to be not defined, but $this->total_recs is... |
|
|
|
|
2. i'm blind, i was looking at those two lines, but i didn't see it.
ignore v5 uploaded, i was too fast
draft documentation uploaded.
v6:
- some $this was missing (when custom buttons)
- extra 'js' argument for custom buttons (will be placed withing < ..>)
- extra js warning for savedelete (please fill that js, all my tries failed...), something like Are you sure to delete ? y/n. but that will require all language files change, so i'm unsure... |
|
|
|
hbernard
|
|
2004-05-17 12:09
|
|
This is what you want :
$js = 'onClick="return confirm(\'sure ?\')"';
return $this->htmlSubmit('savedelete', 'Delete',
$this->getCSSclass('save', $position), false, $enabled ? 0 : $disabled, $js); |
|
|
|
|
another version uploaded (7). the last, i promise....
changes to previous version:
- nav_up() now says whether there are buttons in up (fixed a problem with tabs and ..['up'] = false;)
- ['AC'] syntax was removed, only 'A' is left (due to previous)
- js confirmation of savedelete ( 'Delete ?', no need to change language files)
- extra 'js' parameter for custom submit buttons
- goto_text introduced (a text input field to jump to a page number, as requested in a forum) (needed some js submitting, see the code)
- extra keywords total_pages, current_page, total_recs to output resceptive numbers
- not matched keyword will be translated according to language files
- added extra display options num_pages and num_records (since they can be altered but buttons)
- if too large page number is entered, the last page is displayed
documentation updated as well
please take it away from me... |
|
|
|
hbernard
|
|
2004-05-20 18:01
|
|
Michal, please stop adding feature into feature, because then we will never see the end of this. In fact, this is partially what happen to 'php' feature.
Please. |
|
|
|
|
i promise i won't look at this bug report again :)
anyway, looks working ? |
|
|
|
|
|
I would like to see this commited. What were the difficulties? AFAIR this was nearly before the commit. |
|
|
|
|
|
michal, did you have a time to re-create the patch? |
|
|
|
|
i rebuilt the patch. it's just version 7 patched to most current cvs (166), so documentation can stay the same.
cgi prefixing and renaming of sql function caused the whole problem. |
|
|
|
|
|
michal, nice to hear from you. I will test the patch yesterday (no, not in ten minutes ;-) other developers may test as well. |
|
|
|
|
Commited into the CVS with docs as well.
Please do some testing and we can resolve this after some time. |
|
|
|
|
|
Added into the CVS long time ago. |
|