Update to display the name of the session in the "Delete Session" confirmation dialogue box
AbandonedPublic

Authored by kossebau on Jun 9 2018, 7:16 AM.

Details

Reviewers
brauch
sagnikchaudhuri
Group Reviewers
KDevelop
Summary

Update to display the name of the session in the "Delete Session" confirmation dialogue box before being deleted, as mentioned in this bug (https://bugs.kde.org/show_bug.cgi?id=393138)
Now the "the session is locked by..." message also displays the name of the session.


BEFORE -

BUG: 393138

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
sagnikchaudhuri created this revision.Jun 9 2018, 7:16 AM
Restricted Application added a subscriber: kdevelop-devel. ยท View Herald TranscriptJun 9 2018, 7:16 AM
sagnikchaudhuri requested review of this revision.Jun 9 2018, 7:16 AM
sagnikchaudhuri edited the summary of this revision. (Show Details)Jun 9 2018, 7:19 AM
sagnikchaudhuri edited the summary of this revision. (Show Details)
apol added a subscriber: apol.Jun 14 2018, 3:34 PM
apol added inline comments.
kdevplatform/shell/sessionchooserdialog.cpp
203

Why do you chop(3) there?

228

Discard unrelated change

apol edited reviewers, added: KDevelop; removed: kfunk, mwolff.Jun 14 2018, 3:34 PM
kdevplatform/shell/sessionchooserdialog.cpp
203

Otherwise I noticed it displayed "Hello World: " instead of just "Hello World"

kdevplatform/shell/sessionchooserdialog.cpp
228

Anything else needs to be done?

croick added a subscriber: croick.Jun 20 2018, 9:25 PM

Discard unrelated change

Anything else needs to be done?

Could you reinsert the (additional) trailing line to restrict the commit to what it's supposed to solve?
Also it would be nice to have a short comment in the code, explaining why the last 3 characters have to be removed. Since that is not obvious from the code.

brauch accepted this revision.Jun 21 2018, 7:22 AM
brauch added a subscriber: brauch.

Otherwise looks good, I can submit this later.

kdevplatform/shell/sessionchooserdialog.cpp
218

missing a space after the comma

This revision is now accepted and ready to land.Jun 21 2018, 7:22 AM
brauch added inline comments.Jun 21 2018, 7:33 AM
kdevplatform/shell/sessionchooserdialog.cpp
203

Thinking about it, it would probably be better to introduce an extra role in the item model which only contains the short name, if that is what you want to display. Chopping off 3 characters at the end will not always work I think.

kdevplatform/shell/sessionchooserdialog.cpp
218

So should I upload another diff file containing the missing space, the changes @croick mentioned?
Also, since I don't know how this works, will I get notified when the patch is finally submitted to github?

KDE only uses github as a read-only mirror. You will get a notification when it is submitted.

I don't care much about the newline, I can remove that when commiting; I'd rather think about fixing the other thing I mentioned with stripping off the last 3 characters ...

Can I get an instance when chopping off the last 3 letters won't work? Like, shouldn't the last 3 letters always be ": "
Or will it change if the name of the session's too big or something?

Can I get an instance when chopping off the last 3 letters won't work? Like, shouldn't the last 3 letters always be ": "
Or will it change if the name of the session's too big or something?

That is exactly what I want you to figure out ;)
Yes, I think the string format might depend on whether the session has a name assigned etc

I noticed that if I create a new session and don't name it anything at all. Then I run 'kdevelop --pick-session', that session doesn't show up in the list.
Also, suppose a session is named 'Hello'. It is displayed as 'Hello: ' in the session picker list, the kdevelop window title bar, and also under 'Session' (below 'Start New Session') like in the screenshot.
Should I try to change all of these too? Because that will eliminate the chopping off 3 letters part.

AFAIK that is followed by the names of the projects open in that session, and that is also where your approach with chopping of the chars will probably go wrong.

I am sorry sir. Didn't quite get you...

Try opening a few projects in the session. The names of these projects should follow the colon afterwards.

Okay, so now my objective will be to somehow detect the first colon, and then display everything before that colon, right?
Or should I need to consider anything else?

No, I think you should introduce a way to get the session name before the colon is added. You can introduce a new role for the item model for example, have a look at QStandardItem::setData.

I think I need a few more days since I am just starting to learn Qt. Is that okay?

Of course, nobody is exactly sitting here thinking "eeh, now I've been waiting for that guy for *weeks* to get his patch in" ;)
If you need advice, feel free to ask.

@sagnikchaudhuri: Hi, how are you? Still busy exploring the wonderful world of Qt? :)

There has been no more activity on this WIP patch since June. To keep our list of patches to review clean from dead ones, I would mark this review request as abandoned on Oct. 15th, unless there is an update. After that once there is some new version of the patch, it can be reactivated again.

Still looking forward to see you working on this bug fix again :)

kossebau commandeered this revision.Oct 28 2018, 1:03 PM
kossebau added a reviewer: sagnikchaudhuri.

Abandoning for now given inactivity by the original author, for cleaning the to-review list. Still hoping one day someone/you will pick up this again.

kossebau abandoned this revision.Oct 28 2018, 1:03 PM