Differential D29345

Fixes Dolphin crash on "Defocus Terminal Panel" menu click if no Konsole is installed.
ClosedPublic
Actions

Authored by nikolaik on May 1 2020, 5:55 PM.

Details

Reviewers
meven
elvisangelaccio
ngraham
Group Reviewers
Dolphin
Commits
R318:a9e2f01add83: Fixes Dolphin crash on "Defocus Terminal Panel" menu click if no Konsole is…
R318:2c72b18661f0: Fixes Dolphin crash on "Defocus Terminal Panel" menu click if no Konsole is…
Summary

Dolphin crashes if no Konsole is installed and user clicks menu action "Defocus Terminal Panel".
This fix is pretty straight forward.

Steps to reproduce:

  1. Run Dolphin without Konsole available.
  2. Press F4 to open console window.
  3. Click in service menu "Focus Terminal Panel".
  4. Click in service menu "Defocus Terminal Panel". Observe the crash.
Test Plan
  1. Run Dolphin without Konsole available.
  2. Press F4 to open console window.
  3. Click in service menu "Focus Teminal Panel".
  4. Click in service menu "Defocus Terminal Panel".
  5. Click in service menu "Focus Terminal Panel".
  6. Press F4 to close console window.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nikolaik created this revision.May 1 2020, 5:55 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMay 1 2020, 5:55 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
nikolaik requested review of this revision.May 1 2020, 5:55 PM
meven accepted this revision.May 2 2020, 6:25 AM
This revision is now accepted and ready to land.May 2 2020, 6:25 AM
elvisangelaccio accepted this revision.May 3 2020, 4:01 PM
elvisangelaccio added inline comments.
src/panels/terminal/terminalpanel.cpp
313

else not needed after a return.

nikolaik updated this revision to Diff 81820.May 3 2020, 7:00 PM

Removed unnecessary 'else' keyword.

nikolaik marked an inline comment as done.May 3 2020, 7:02 PM
nikolaik edited the summary of this revision. (Show Details)
nikolaik edited the test plan for this revision. (Show Details)
meven added a comment.May 4 2020, 8:39 AM

I wonder is the issue is that F4 was available in the first place.

src/panels/terminal/terminalpanel.cpp
315 ↗(On Diff #81820)

Perhaps return false.

nikolaik added a comment.May 4 2020, 9:39 AM
In D29345#662670, @meven wrote:

I wonder is the issue is that F4 was available in the first place.

This is a question.
As i can see without F4 we won't be able to see a message about Konsole not been installed. For example, no need to show terminal panel at all, we can pop error message.
Looks like idea was to show the panel with a message in it or with a Konsole. In that case F4 in necessary and focus in/out is still necessary, and we should return panel focus (not return false).
This additions comes from D10959.
@ngraham what do you think?

ngraham accepted this revision.May 4 2020, 3:42 PM

Yes the idea was to keep the action visible to expose the feature for those who don't have Konsole installed. This seems sane to me.

ngraham added a comment.May 4 2020, 3:43 PM

I would recommend landing on the stable branch.

Closed by commit R318:a9e2f01add83: Fixes Dolphin crash on "Defocus Terminal Panel" menu click if no Konsole is… (authored by nikolaik). · Explain WhyMay 4 2020, 7:36 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 81936

View Options

src/panels/terminal/terminalpanel.cpp

Loading...