DevHeads.net

Review Request: Implement a dialog to show unhandled Bugzilla errors

Review request for KDE Runtime and George Kiagiadakis.

Summary
Initial patch

I created a dialog that will show the RAW Bugzilla HTML output when the response can't be parsed by DrKonqi. The user is encouraged to perform the action again later or save the HTML data to submit a DrKonqi bug.

This function is available for the login and the submit (new report or attach to an existant one) processes.
BugzillaLib error message signals were enhanced to hold the raw html (if it makes sense to show it)

The text on the dialog could be changed.

Additional fix: disconnect signals prior to creating a new report or attaching to an existing one; avoiding duplicates dialog. (may be this should go as another patch)

This addresses bug 266542.
<a href="http://bugs.kde.org/show_bug.cgi?id=266542" title="http://bugs.kde.org/show_bug.cgi?id=266542">http://bugs.kde.org/show_bug.cgi?id=266542</a>

Diffs
drkonqi/CMakeLists.txt 87c3d24
drkonqi/bugzillalib.h cb7da10
drkonqi/bugzillalib.cpp 348fb72
drkonqi/reportassistantpages_bugzilla.h 8681f22
drkonqi/reportassistantpages_bugzilla.cpp e896c52
drkonqi/reportinterface.h a1fc78a
drkonqi/reportinterface.cpp a484d11

Diff: <a href="http://git.reviewboard.kde.org/r/100681/diff" title="http://git.reviewboard.kde.org/r/100681/diff">http://git.reviewboard.kde.org/r/100681/diff</a>

Testing
Tested by faking some errors during the login and submit (new report) processes.
The code should be reviewed as I stop coding for KDE several months ago.

Screenshots
Unhandled Error Dialog
<a href="http://git.reviewboard.kde.org/r/100681/s/77/" title="http://git.reviewboard.kde.org/r/100681/s/77/">http://git.reviewboard.kde.org/r/100681/s/77/</a>

Thanks,

Darío Andrés

Comments

Re: Review Request: Implement a dialog to show unhandled Bugzill

By =?utf-8?b?RGFyw... at 02/19/2011 - 07:33

(Updated Feb. 19, 2011, 12:33 p.m.)

Review request for KDE Runtime and George Kiagiadakis.

Changes
New version of the patch.
- Fixed errors mentioned by George
- Added updated copyright (2011)
- Removed some code that doesn't belong to this patch (but it should be commited later)

George: can you locally test this patch and check if it compiles properly ? I don't have a trunk environment to test so I had to use a modified CMakeLists.txt on drkonqi/ to test this.

Thanks!

Summary
Initial patch

I created a dialog that will show the RAW Bugzilla HTML output when the response can't be parsed by DrKonqi. The user is encouraged to perform the action again later or save the HTML data to submit a DrKonqi bug.

This function is available for the login and the submit (new report or attach to an existant one) processes.
BugzillaLib error message signals were enhanced to hold the raw html (if it makes sense to show it)

The text on the dialog could be changed.

Additional fix: disconnect signals prior to creating a new report or attaching to an existing one; avoiding duplicates dialog. (may be this should go as another patch)

This addresses bug 266542.
<a href="http://bugs.kde.org/show_bug.cgi?id=266542" title="http://bugs.kde.org/show_bug.cgi?id=266542">http://bugs.kde.org/show_bug.cgi?id=266542</a>

Diffs (updated)
drkonqi/CMakeLists.txt 87c3d24
drkonqi/bugzillalib.h cb7da10
drkonqi/bugzillalib.cpp 348fb72
drkonqi/reportassistantdialog.cpp 607dd63
drkonqi/reportassistantpages_bugzilla.h 8681f22
drkonqi/reportassistantpages_bugzilla.cpp e896c52
drkonqi/reportinterface.h a1fc78a
drkonqi/reportinterface.cpp a484d11

Diff: <a href="http://git.reviewboard.kde.org/r/100681/diff" title="http://git.reviewboard.kde.org/r/100681/diff">http://git.reviewboard.kde.org/r/100681/diff</a>

Testing
Tested by faking some errors during the login and submit (new report) processes.
The code should be reviewed as I stop coding for KDE several months ago.

Screenshots
Unhandled Error Dialog
<a href="http://git.reviewboard.kde.org/r/100681/s/77/" title="http://git.reviewboard.kde.org/r/100681/s/77/">http://git.reviewboard.kde.org/r/100681/s/77/</a>

Thanks,

Darío Andrés

Re: Review Request: Implement a dialog to show unhandled Bugzill

By =?utf-8?b?RGFyw... at 03/07/2011 - 15:29

(Updated March 7, 2011, 8:29 p.m.)

Review request for KDE Runtime and George Kiagiadakis.

Changes
Finally got a KDE trunk installation again. The second version of the patch didn't even compile.
This new version compiles and check the QWeakPointer validation.
Note: this validation should also be applied on drkonqi.cpp (DrKonqi::saveReport)

Summary
Initial patch

I created a dialog that will show the RAW Bugzilla HTML output when the response can't be parsed by DrKonqi. The user is encouraged to perform the action again later or save the HTML data to submit a DrKonqi bug.

This function is available for the login and the submit (new report or attach to an existant one) processes.
BugzillaLib error message signals were enhanced to hold the raw html (if it makes sense to show it)

The text on the dialog could be changed.

Additional fix: disconnect signals prior to creating a new report or attaching to an existing one; avoiding duplicates dialog. (may be this should go as another patch)

This addresses bug 266542.
<a href="http://bugs.kde.org/show_bug.cgi?id=266542" title="http://bugs.kde.org/show_bug.cgi?id=266542">http://bugs.kde.org/show_bug.cgi?id=266542</a>

Diffs (updated)
drkonqi/CMakeLists.txt 87c3d24
drkonqi/bugzillalib.h cb7da10
drkonqi/bugzillalib.cpp 348fb72
drkonqi/reportassistantpages_bugzilla.h 8681f22
drkonqi/reportassistantpages_bugzilla.cpp e896c52
drkonqi/reportinterface.h a1fc78a
drkonqi/reportinterface.cpp a484d11

Diff: <a href="http://git.reviewboard.kde.org/r/100681/diff" title="http://git.reviewboard.kde.org/r/100681/diff">http://git.reviewboard.kde.org/r/100681/diff</a>

Testing
Tested by faking some errors during the login and submit (new report) processes.
The code should be reviewed as I stop coding for KDE several months ago.

Screenshots
Unhandled Error Dialog
<a href="http://git.reviewboard.kde.org/r/100681/s/77/" title="http://git.reviewboard.kde.org/r/100681/s/77/">http://git.reviewboard.kde.org/r/100681/s/77/</a>

Thanks,

Darío Andrés

Re: Review Request: Implement a dialog to show unhandled Bugzill

By Commit Hook at 04/29/2011 - 09:29

This review has been submitted with commit b3aacf5147e4a47b3ba7433648cb69b102e777a8 by Dario Andres Rodriguez.

- Commit

On March 7, 2011, 8:29 p.m., Darío Andrés Rodríguez wrote:

Re: Review Request: Implement a dialog to show unhandled Bugzill

By Matthias Fuchs at 02/21/2011 - 05:04

drkonqi/reportassistantpages_bugzilla.cpp
<http://git.reviewboard.kde.org/r/100681/#comment1329>

You have to check if the pointer is still valid there.

The reason for using QPointer or QWeakPointer in the first place is that the dialog could be destroyed by e.g. a dbus command IIRC --> there was a blog entry on how to crash nearly every KDE app which commented on that.

So it could be that it is not valid once it reaches the KUrl fileUrl = ... line.
The only way to check if it is valid is using QWeakPointer or QPointer and do KUrl fileUrl (dlg ? dlg->selectedUrl() : KUrl());

Yes I suppose in the worst case it could be deleted inbetween but that should not happen that often. ;)

- Matthias

On Feb. 19, 2011, 12:33 p.m., Darío Andrés Rodríguez wrote:

Re: Review Request: Implement a dialog to show unhandled Bugzill

By George Kiagiadakis at 02/19/2011 - 06:21

Ship it!

Hey, thanks for the patch. I didn't expect you to suddenly appear and implement this in such a short time, but I am glad that you did. I only have some minor comments to make. You know this code better than me anyway, so I trust it does the right thing.

drkonqi/reportassistantpages_bugzilla.cpp
<http://git.reviewboard.kde.org/r/100681/#comment1270>

Why i18nc with an empty context? i18n("Save to a file") should work.

drkonqi/reportassistantpages_bugzilla.cpp
<http://git.reviewboard.kde.org/r/100681/#comment1272>

Is there a specific reason to use QPointer here? KFileDialog* would also work fine. I am mostly saying this because QPointer is kind of deprecated in favor of QWeakPointer now...

drkonqi/reportassistantpages_bugzilla.cpp
<http://git.reviewboard.kde.org/r/100681/#comment1271>

There is an unecessary extra space in the translated string here.

- George

On Feb. 19, 2011, 12:34 a.m., Darío Andrés Rodríguez wrote:

Re: Review Request: Implement a dialog to show unhandled Bugzill

By =?utf-8?b?RGFyw... at 02/19/2011 - 07:33

Forgot that one

QPointer is also used in DrKonqi::saveReport (drkonqi.cpp). In fact I only copied the code and modified it a bit.
I have changed my code to QWeakPointer

Little error, removed too.

- Darío Andrés

On Feb. 19, 2011, 12:33 p.m., Darío Andrés Rodríguez wrote: