Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix 4415,4453,4452,4451; RFE-1518 (Navigate away) improvements #1222

Merged
merged 1 commit into from Jun 13, 2014

Conversation

chirayuchiripal
Copy link
Contributor

-> Reverted old partial fix for bug-4415
-> Complete Fix for Bug-4415 + "Filter Rows:" prompt fix
-> It now considers only input/textarea which are inside form elements. (This fixes unnecessary prompts on "Filter rows:" of browse tab).

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a5fe8ea on D-storm:bug-4415 into a6d379c on phpmyadmin:master.

@chirayuchiripal chirayuchiripal changed the title Bug 4415: about leaving the page after updating row data from browse tab; and filter rows prompt fix Bugfix 4415; RFE-1518 (Navigate away) improvements Jun 12, 2014
@chirayuchiripal
Copy link
Contributor Author

-> Now, listens only on forms with "lock-page" class.
-> Added lock-page class to SQL tab, Search tab (table level), Insert Tab, create table interface.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 7385c28 on D-storm:bug-4415 into a7bc3a7 on phpmyadmin:master.

@chirayuchiripal
Copy link
Contributor Author

-> Displays lock icon at top right corner (just before "goto pagetop") in menubar when form gets edited.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling d9a4e54 on D-storm:bug-4415 into d909aa6 on phpmyadmin:master.

@chirayuchiripal
Copy link
Contributor Author

-> Complete makeover
-> bugfix 4453/52/51

@chirayuchiripal chirayuchiripal changed the title Bugfix 4415; RFE-1518 (Navigate away) improvements Bugfix 4415,4453,4452,4451; RFE-1518 (Navigate away) improvements Jun 13, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling b80a41a on D-storm:bug-4415 into d909aa6 on phpmyadmin:master.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling de0f7bc on D-storm:bug-4415 into * on phpmyadmin:master*.

@lem9 lem9 self-assigned this Jun 13, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling de0f7bc on D-storm:bug-4415 into fe93f9e on phpmyadmin:master.

@chirayuchiripal
Copy link
Contributor Author

Thanks for the github example, maybe now I can explain it in a better way.

CASE 1 (No changes, no prompt on cancel):
1] Click "Edit Comment" link on PR.
2] Dont change anything and click "cancel".
3] No prompt.

CASE 2 (Unsaved changes, prompt on cancel):
1] Click "Edit Comment" link on PR.
2] Change some content and click "cancel".
3] Prompts "Are you sure you want to cancel? You have unsaved changes that will be reverted".

CASE 3 (Revert changes, no prompt on cancel): [Bug-4452]
1] Click "Edit Comment" link on PR.
2] Change some content and revert them.
3] click "cancel".
4] No prompt.

CASE 4 (Truncating existing data, prompt on cancel): [Bug-4453]
1] Click "Edit Comment" link on PR.
2] truncate the field and click "cancel".
3] Prompts "Are you sure you want to cancel? You have unsaved changes that will be reverted".

CASE 5 (cut/pasting using mouse, prompt on cancel): [Bug-4451]
1] Click "Edit Comment" link on PR.
2] right-click and paste some content and click "cancel".
3] Prompts "Are you sure you want to cancel? You have unsaved changes that will be reverted".

@chirayuchiripal
Copy link
Contributor Author

I think we should also use similar prompt message. How about this one: "Are you sure you want to leave this page? You have unsaved changes that will be reverted." or any other suggestion would be appreciated.

@lem9
Copy link
Contributor

lem9 commented Jun 13, 2014

I suggest "You have unsaved changes; are you sure you want to leave this page?".

@chirayuchiripal
Copy link
Contributor Author

That is also a good one.

@mebjas
Copy link
Contributor

mebjas commented Jun 13, 2014

Well, I was considering case of editing any file in github, they do not take care if the values have been reverted, the prompt is triggered for any change. Although its a good feature to have if saving the values of each fields in not an overhead!

But considering the one, when we truncate the values. If we look at the scenario for PMA, If I have empty fields, I do not have any data to loose.. in this case, I think maximum probability is user intentionally want to navigate away and thus should not get a prompt. Even if false negative occurs, the user has no data to loose!

@chirayuchiripal
Copy link
Contributor Author

@mebjas, after reverting the changes whether we save or not does not make the difference so there is no need to prompt and as far as saving values of each field is considered then it is not an overhead because I am not saving the values of each field, here I am hashing the old value and saving that hash inside data-attribute of form element which is fixed small size data no matter how big your data is.

And considering the data loss only is not good, we should consider changes because deletions are equally important to additions. It may be possible that user wants to empty the field and accidentally navigates away and his/her changes will not get saved. Suppose a user is editing a row (insert/edit tab) having 100 columns all full of data and you want to empty the 90 columns of it. After doing this, (s)he accidentally clicked a link then it would get navigated away without saving it. Yes, if the field initially was empty then it will not prompt the user.

@mebjas
Copy link
Contributor

mebjas commented Jun 13, 2014

Well I think this condition will arise in mostly in editing tab only. So we can keep it.

@chirayuchiripal
Copy link
Contributor Author

Exactly.

Signed-off-by: Chirayu Chiripal <chirayu.chiripal@gmail.com>
@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@lem9
Copy link
Contributor

lem9 commented Jun 13, 2014

I have started testing and so far, I like this patch. Can you confirm that grid editing with the following case does not lock the page?

$cfg['SaveCellsAtOnce'] = true;

@chirayuchiripal
Copy link
Contributor Author

Currently, I have not added lock-page class to grid editing form. So, it will not work now. If it is needed to add there, I can add it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling b527dc7 on D-storm:bug-4415 into 239f4c0 on phpmyadmin:master.

@lem9
Copy link
Contributor

lem9 commented Jun 13, 2014

Well it can wait; I'll merge this pull request first, after my tests.

@lem9
Copy link
Contributor

lem9 commented Jun 13, 2014

Nice work. I'm not sure we need to lock the table Search page (at least, leaving this page would not cause much harm) but I'm happy to leave it locked for now.

lem9 added a commit that referenced this pull request Jun 13, 2014
Bugfix 4415,4453,4452,4451;  RFE-1518 (Navigate away) improvements
@lem9 lem9 merged commit c607dad into phpmyadmin:master Jun 13, 2014
@chirayuchiripal chirayuchiripal deleted the bug-4415 branch June 16, 2014 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants