Bug 183069 - Crash on removing key bindings
Summary: Crash on removing key bindings
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: general (show other bugs)
Version: 2.2
Platform: unspecified Linux
: NOR crash (vote)
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
: 227594 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-02-03 23:11 UTC by Albert Astals Cid
Modified: 2010-02-24 05:28 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Proposed patch (1.32 KB, patch)
2009-05-25 02:00 UTC, Dario Andres
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Albert Astals Cid 2009-02-03 23:11:18 UTC
Version:           2.2 (using 4.2.00 (KDE 4.2.0), Kubuntu packages)
Compiler:          cc
OS:                Linux (x86_64) release 2.6.27-11-generic

How to reproduce:
 * Settings
 * Edit current profile
 * Input
 * Edit
 * Select two rows
 * Press remove

#4  QTableWidget::row (this=0x57000000e9, item=0x332f940) at ../../include/QtGui/private/../../../src/gui/itemviews/qtablewidget_p.h:189
#5  0x00007f1a5f28e658 in QTableWidgetItem::row (this=0x332f940) at /home/kde42/qt44/include/QtGui/../../src/gui/itemviews/qtablewidget.h:356
#6  0x00007f1a5f2b256d in Konsole::KeyBindingEditor::removeSelectedEntry (this=0x3315fd0) at /home/kde42/kdebase/apps/konsole/src/KeyBindingEditor.cpp:78
Comment 1 Dario Andres 2009-05-25 01:12:24 UTC
I can reproduce if I select the two columns of the same row.

_ui->keyBindingTable->selectedItems() will return both items, but the deletion process removes the whole row at the end, so when the loop is going to process the second item, its row is gone and it will crash.

Possible solutions:
- Filter the "_ui->keyBindingTable->selectedItems()" list to only have one item per row.
- Do any other check to not access that item if its row does not exist
Comment 2 Dario Andres 2009-05-25 02:00:31 UTC
Created attachment 33986 [details]
Proposed patch

Filter the table items to get the unique ones (and column==0)

I also tried filtering the items in the original iteration loop but it caused errors as we were accessing to dead/invalid elements.

I hope indentantion and styles are OK
Comment 3 Dario Andres 2009-05-25 19:51:20 UTC
I can commit the patch if it gets reviewed and approved...
Comment 4 Robert Knight 2009-05-26 11:11:04 UTC
Hi Dario,

Thanks for the patch - I think it would be simpler just to remove any items from the list returned by selectedItems() which are not in the first column before iterating over each item in the list to remove the rows.

Alternatively, iterate through the selected items to create a set of selected rows then iterate through that set to remove the rows.
Comment 5 Dario Andres 2009-05-27 19:10:07 UTC
(In reply to comment #4)
> Hi Dario,
> 
> Thanks for the patch - I think it would be simpler just to remove any items
> from the list returned by selectedItems() which are not in the first column
> before iterating over each item in the list to remove the rows.
> 

That could work if you set the selection mode to the whole row. Otherwise, user could select a binding (2° column) and it won't be deleted (as the 1° column is not selected)

> Alternatively, iterate through the selected items to create a set of selected
> rows then iterate through that set to remove the rows.

That is what my patch is doing: creating a new list of unique rows items and then iterate normally.

Regards
Comment 6 Kurt Hindenburg 2010-01-24 17:22:20 UTC
I fixed the issue where clicking will select the entire row in bug 183070.
Comment 7 Kurt Hindenburg 2010-01-30 15:25:20 UTC
SVN commit 1082422 by hindenburg:

Fix crash on removing key bindings.  Patch by  Dario Andres

BUG: 183069


 M  +16 -2     KeyBindingEditor.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1082422
Comment 8 Dario Andres 2010-02-19 23:26:22 UTC
Thanks for fixing. - Is the fix going to be backported ? Regards
Comment 9 Dario Andres 2010-02-19 23:27:06 UTC
*** Bug 227594 has been marked as a duplicate of this bug. ***
Comment 10 Kurt Hindenburg 2010-02-24 05:28:46 UTC
SVN commit 1095365 by hindenburg:

Fix crash on removing key bindings.  Patch by  Dario Andres

CCBUG: 183069


 M  +16 -2     KeyBindingEditor.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1095365