Add profile support interface for TerminalInterface
ClosedPublic

Authored by mschiller on Apr 1 2019, 3:22 PM.

Details

Summary

Extend the interface of kde_terminal_interface in order to
support: Getting the available Profiles, Getting the Name
to the currently active profile and changing the active
profile to one of the profiles returned by getAvailabaleProfiles().
Furthermore querying the active profile for properties.

Diff Detail

Repository
R306 KParts
Branch
terminal-interface-profiles (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11085
Build 11103: arc lint + arc unit
mschiller created this revision.Apr 1 2019, 3:22 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 1 2019, 3:22 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mschiller requested review of this revision.Apr 1 2019, 3:22 PM

Can someone from the frameworks group clarify if this can be committed for the next release 5.58 next month? I know years ago, we had a TerminalInterfaceV2 because the interface was changed.

https://community.kde.org/Schedules/Frameworks

mschiller edited the summary of this revision. (Show Details)Apr 21 2019, 6:35 PM
pino added a subscriber: pino.Apr 21 2019, 6:38 PM

Yes, this change breaks the binary compatibility. Please add a TerminalInterfaceV2 that inherits TerminalInterface instead, or add a separate interface for profile handling.

mschiller updated this revision to Diff 56672.Apr 21 2019, 6:47 PM
mschiller edited the summary of this revision. (Show Details)

Move the profile support to different class

pino added a comment.Apr 21 2019, 7:38 PM

General notes on the API (not a konsole developer though):

  • an abstract virtual destructor is needed
  • in kdelibs/kf5 lingo, the "get" as prefix of API getters is generally not used
  • setCurrentProfile instead of changeCurrentProfile? the latter makes me think it does changes to the current profile, rather than setting another profile as current
  • wrt changeCurrentProfile: what will it do if the specified profile name does not exist? should the API return true/false to indicate the switch actually succeeded?
  • wrt getProfilePath: what if an application does not store a profile as single filename, for example as directory, or even stored e.g. in a DB? maybe it would be better to have this as property of a profile
  • is a setProfileProperty worth having?
  • is an API to list the available properties worth having?

Also: this file is still not installed, so it cannot be used. Maybe just add the new interface to the existing kde_terminal_interface.h?

src/terminalprofileinterface.h
2

either put the right filename, or just remove (IMHO better)

3

this is certainly yours now

20

I'd go with KPARTS_etc (instead of KDELIBS_etc)

wrt getProfilePath: what if an application does not store a profile as single filename, for example as directory, or even stored e.g. in a DB? maybe it would be better to have this as property of a profile

Yeah Konsole Profiles seem to already have a path property in the profile implementation. So gonna remove this.

is a setProfileProperty worth having?

I don't think it would be good to have the user applications mess with the users customizations. In case the profile is somehow "incompatible" with the client application, exposing some profile management functions (e.g. copy and modify) might be useful but I can't think of an actual reason which would require that.

is an API to list the available properties worth having?

Just to check at runtime if getting a property was successful returning an empty QVariant should be enough, because you need to look up the available properties to check the returned type anyway.

Please fix the API getters: in the KDE/Qt world, getters never start with 'get': just use availableProdiles() etc.

Similarly, I suggest to rename changeCurrentProfile() to setCurrentProfile().

Could you provide an updated patch?

mschiller updated this revision to Diff 56696.Apr 22 2019, 7:36 AM

Remove getProfilePath and remove get prefixes

mschiller marked 3 inline comments as done.Apr 22 2019, 7:37 AM
mschiller updated this revision to Diff 56697.Apr 22 2019, 7:39 AM

change KDEPARTS to KPARTS

@hindenburg: lgtm, could you comment?

I would prefer using TerminalInterfaceV2 in the current file instead of a new file. It would make using it much easier to use, document, etc and when kf6 comes out, easier to re-combine.

mschiller updated this revision to Diff 58094.May 14 2019, 5:39 PM

Move the interface to TerminalInterfaceV2

mschiller updated this revision to Diff 58095.May 14 2019, 5:41 PM
  • Remove install of profileinterface

ping

I was waiting until the next release of frameworks is out.

5.59 Sat June 1st, 2019 (expected) Sat June 8th, 2019

Oh sry, I wasn't aware of that.

hindenburg edited the summary of this revision. (Show Details)Jun 11 2019, 12:38 AM
hindenburg accepted this revision.Jun 11 2019, 12:50 AM
This revision is now accepted and ready to land.Jun 11 2019, 12:50 AM
This revision was automatically updated to reflect the committed changes.