D6727: Destroy all kwayland objects created by registry when it is destroyed
davidedmundson created this revision. Restricted Application added subscribers: Frameworks, plasma-devel. Restricted Application added projects: Plasma on Wayland, Frameworks. REVISION SUMMARY Currently one has to connect every object manually to connectionDied, which is something we can do for them. If the user also has a connection, the second will just no-op. Will add some awesome docs / unit tests if you're into the idea. TEST PLAN Commit 2: Emit connectionDied if the QPA owns the connection and is destroyed. We have a few objects that linger longer than the qApp. I've got a crash in plasmashell, and I've had a crash with breeze wrapping a dangly menu in konversation. This should fix those. REPOSITORY R127 KWayland BRANCH davidedmundson/xdgv6 REVISION DETAIL https://phabricator.kde.org/D6727 AFFECTED FILES src/client/connection_thread.cpp src/client/output.cpp src/client/output.h src/client/outputdevice.cpp src/client/outputdevice.h src/client/registry.cpp src/client/registry.h To: davidedmundson Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6683: [server] Send keyboard leave when client destroys the focused surface
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R127 KWayland BRANCH keyboard-leave-surface-destroy REVISION DETAIL https://phabricator.kde.org/D6683 To: graesslin, #frameworks, #plasma, davidedmundson Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6683: [server] Send keyboard leave when client destroys the focused surface
graesslin updated this revision to Diff 16755. graesslin added a comment. Restricted Application edited projects, added Plasma; removed Plasma on Wayland. Add nullptr check to prevent crash REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6683?vs=16661=16755 BRANCH keyboard-leave-surface-destroy REVISION DETAIL https://phabricator.kde.org/D6683 AFFECTED FILES autotests/client/test_wayland_seat.cpp src/server/keyboard_interface.cpp src/server/resource.cpp src/server/resource.h To: graesslin, #frameworks, #plasma Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6683: [server] Send keyboard leave when client destroys the focused surface
graesslin planned changes to this revision. graesslin added a comment. Causes crash in KWayaland when closing a window. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D6683 To: graesslin, #frameworks, #plasma Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
elvisangelaccio added a comment. In https://phabricator.kde.org/D6709#125694, @thiago wrote: > This class isn't hooked up to anything. It's technically correct as an FD sender and receiver. What I want to see is how you use it, because that's extremely important to get right. I think he's going to upload another patch that uses this code. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
thiago added a comment. This class isn't hooked up to anything. It's technically correct as an FD sender and receiver. What I want to see is how you use it, because that's extremely important to get right. I can confirm to you that anonymous namespace sockets do not work on BSDs (which include macOS). INLINE COMMENTS > sharefd.cpp:112 > + > +m_socketDes = ::socket(AF_LOCAL, SOCK_STREAM, 0); > +if (m_socketDes == -1) If SOCK_NONBLOCK is defined, OR it o SOCK_STREAM and then you don't have call setNonBlocking below. > sharefd.cpp:117 > +KSockaddrUn addr(path); > +bool binded = bind(m_socketDes, addr.address(), addr.length()) != -1; > +bool listening = listen(m_socketDes, 5) != -1; "binded" is not English. You mean "bound". > sharefd.h:33 > + > +bool startListening(const std::string ); > +int fileDescriptor() const; Use QString, not std::string. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6197: Add basic KAuth support to file ioslave
chinmoyr added a subscriber: davidedmundson. chinmoyr added a comment. @davidedmundson will this revision solve the problem you mentioned https://phabricator.kde.org/D6198. I will include the code for warning in the job itself. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6197 To: chinmoyr, elvisangelaccio, #frameworks, dfaure Cc: davidedmundson, dfaure, eliasp, aacid
D6197: Add basic KAuth support to file ioslave
chinmoyr updated this revision to Diff 16738. chinmoyr marked 4 inline comments as done. chinmoyr edited the test plan for this revision. chinmoyr added a comment. In my previous revision the logic for showing warning from ioslave was flawed. In case of deleteRecursive everything would have worked out fine but in case of copy the logic for warning would have failed. Since CopyJob creates number of sub jobs, there are as many number of slaves. If there happen to be more than one file with read access restricted then ioslave's warning would have been shown multiple times. In this revision, I added a variable `m_enablePrivilegeExecution`, a public method `isPrivilegeExecutionEnabled` and an additional job flag `PrivilegeExecution` to the KIO Job class. Now if an application want's to execute a privilege file operation, it will 1. create a job with `PrivilegeExecution` flag. 2. the flag will cause the job to set `m_enablePrivilegeExecution` to true. 3. when `execWithElevatedPrivilege()` is called it will first emit `dataReq()` signal. 4. the job will respond with a message "ElevatePrivilege" if it supports it and the slave will continue. The step 2 is very important. Even if the flag is set its upto us to decide which job should support it. And the jobs which support it will also show warnings prior to notifying the slave. This prevents misuse of the job during the brief period of elevated privilege. @dfaure what do you say about the feasibility of this approach? REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6197?vs=15729=16738 REVISION DETAIL https://phabricator.kde.org/D6197 AFFECTED FILES src/core/job.cpp src/core/job_base.h src/core/job_p.h src/core/simplejob.cpp src/core/simplejob.h src/ioslaves/file/CMakeLists.txt src/ioslaves/file/file.h src/ioslaves/file/file_unix.cpp src/ioslaves/file/file_win.cpp src/ioslaves/file/kauth/CMakeLists.txt src/ioslaves/file/kauth/file.actions src/ioslaves/file/kauth/filehelper.cpp src/ioslaves/file/kauth/filehelper.h To: chinmoyr, elvisangelaccio, #frameworks, dfaure Cc: dfaure, eliasp, aacid
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/ --- (Updated July 15, 2017, 12:53 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 0296b89714ce96b247322d9be09b387573d79193 by David Faure on behalf of Simone Gaiarin to branch master. Bugs: 359233 https://bugs.kde.org/show_bug.cgi?id=359233 Repository: kio Description --- The current "open with" dialog implementation does not follow the KDE principle "Simple by default, powerful when needed" for the following reasons: - The "run in terminal" and "keep terminal open" options are advanced options and should not be exposed by default - The primary goal of the dialog should be to select an application from the app tree, running command is an advanced feature My patch changes the behavior as follow: - Put the two options in a KCollapsibleComboBox collapsed by default - The user can expand it only if he needs to use a command line command Implementation details: - When the KCollapsibleComboBox is clicked it is expanded upward keeping the dialog size fixed and compressing the treeview, I'm not sure this is the best approach, but to make it expand downwards we need to fix the size of the dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be desiderable. Maybe there is a way to keep the dialog resizable and expand the combobox downwards, but I couldn't find it. - I've increased the vertical size of the dialog (which I think it was too small) also to accomodate the upward expansion which otherwise would make the app tree almost disappear Relevant discussions: https://bugs.kde.org/show_bug.cgi?id=359233 https://forum.kde.org/viewtopic.php?f=285=131014 Diffs - src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 Diff: https://git.reviewboard.kde.org/r/130180/diff/ Testing --- Thanks, Simone Gaiarin
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr marked an inline comment as done. chinmoyr added inline comments. INLINE COMMENTS > davidedmundson wrote in sharefd.cpp:107 > If this is on the client side, what stops any other (non authorised) client > listening to here? It doesn't matter. The job of client is to send the file descriptor and any other (unauthorised) client connected cannot see this fd. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
> On July 15, 2017, 9:31 a.m., Simone Gaiarin wrote: > > Inviala! > > Luigi Toscano wrote: > You can't "ship it" you own review. "Ship it" is marked by other > reviewers who thinks that this should go in. > Do you mean to say that you don't have a developer account and someone > else should push this? > > Simone Gaiarin wrote: > I'm not very familiar with review board. Indeed I don't have a developer > account. I thought that once approved by clicking ship it it would go > through, but then I realized what "ship it" is for, so I was just waiting for > someone to push it. So yes, I need someone with a developer account to push > it please. > > David Faure wrote: > I can take care of it, but I need your email address for the git > authorship information. See linked bug report. - Christoph --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/#review103453 --- On July 12, 2017, 8:54 p.m., Simone Gaiarin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130180/ > --- > > (Updated July 12, 2017, 8:54 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 359233 > https://bugs.kde.org/show_bug.cgi?id=359233 > > > Repository: kio > > > Description > --- > > The current "open with" dialog implementation does not follow the KDE > principle "Simple by default, powerful when needed" for the following reasons: > - The "run in terminal" and "keep terminal open" options are advanced options > and should not be exposed by default > - The primary goal of the dialog should be to select an application from the > app tree, running command is an advanced feature > > My patch changes the behavior as follow: > > - Put the two options in a KCollapsibleComboBox collapsed by default > - The user can expand it only if he needs to use a command line command > > > Implementation details: > - When the KCollapsibleComboBox is clicked it is expanded upward keeping the > dialog size fixed and compressing the treeview, I'm not sure this is the best > approach, but to make it expand downwards we need to fix the size of the > dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be > desiderable. Maybe there is a way to keep the dialog resizable and expand the > combobox downwards, but I couldn't find it. > - I've increased the vertical size of the dialog (which I think it was too > small) also to accomodate the upward expansion which otherwise would make the > app tree almost disappear > > > Relevant discussions: > https://bugs.kde.org/show_bug.cgi?id=359233 > https://forum.kde.org/viewtopic.php?f=285=131014 > > > Diffs > - > > src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 > > Diff: https://git.reviewboard.kde.org/r/130180/diff/ > > > Testing > --- > > > Thanks, > > Simone Gaiarin > >
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a comment. And In https://phabricator.kde.org/D6709#125610, @davidedmundson wrote: > > The sequence would be, registering service in ioslave, setting euid of the helper process and sending the file descriptor over user's session bus > > I don't fully know this code, but that doesn't sound right. > > Your helper is running on the system bus, and shouldn't really have access to the user's session bus at all. > Your client side ioslave will have connections to both busses, but only be talking to the helper on the system bus. You can set the effective uid of the root process to the users process to connect to user's session bus and then reset it and gain back the privilege. > With polkit you currently request something over DBus from a helper and you get a reply back. What you're describing you want is exactly that, except the reply happens a file descriptor. (which as you hint is a native DBus type) Why do you need to register a service in the ioslave? QDBusUnixFileDescriptor object is required to send a file descriptor over dbus. The service must have a method accepting QDBusUnixFileDescriptor as an argument, QtDBus won't create it automatically. KAuth doesn't have any such method thats why the need for a separate service. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
> On July 15, 2017, 7:31 a.m., Simone Gaiarin wrote: > > Inviala! > > Luigi Toscano wrote: > You can't "ship it" you own review. "Ship it" is marked by other > reviewers who thinks that this should go in. > Do you mean to say that you don't have a developer account and someone > else should push this? > > Simone Gaiarin wrote: > I'm not very familiar with review board. Indeed I don't have a developer > account. I thought that once approved by clicking ship it it would go > through, but then I realized what "ship it" is for, so I was just waiting for > someone to push it. So yes, I need someone with a developer account to push > it please. I can take care of it, but I need your email address for the git authorship information. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/#review103453 --- On July 12, 2017, 6:54 p.m., Simone Gaiarin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130180/ > --- > > (Updated July 12, 2017, 6:54 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 359233 > https://bugs.kde.org/show_bug.cgi?id=359233 > > > Repository: kio > > > Description > --- > > The current "open with" dialog implementation does not follow the KDE > principle "Simple by default, powerful when needed" for the following reasons: > - The "run in terminal" and "keep terminal open" options are advanced options > and should not be exposed by default > - The primary goal of the dialog should be to select an application from the > app tree, running command is an advanced feature > > My patch changes the behavior as follow: > > - Put the two options in a KCollapsibleComboBox collapsed by default > - The user can expand it only if he needs to use a command line command > > > Implementation details: > - When the KCollapsibleComboBox is clicked it is expanded upward keeping the > dialog size fixed and compressing the treeview, I'm not sure this is the best > approach, but to make it expand downwards we need to fix the size of the > dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be > desiderable. Maybe there is a way to keep the dialog resizable and expand the > combobox downwards, but I couldn't find it. > - I've increased the vertical size of the dialog (which I think it was too > small) also to accomodate the upward expansion which otherwise would make the > app tree almost disappear > > > Relevant discussions: > https://bugs.kde.org/show_bug.cgi?id=359233 > https://forum.kde.org/viewtopic.php?f=285=131014 > > > Diffs > - > > src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 > > Diff: https://git.reviewboard.kde.org/r/130180/diff/ > > > Testing > --- > > > Thanks, > > Simone Gaiarin > >
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
davidedmundson added a comment.
> The sequence would be, registering service in ioslave, setting euid of the
helper process and sending the file descriptor over user's session bus
I don't fully know this code, but that doesn't sound right.
Your helper is running on the system bus, and shouldn't really have access to
the user's session bus at all.
Your client side ioslave will have connections to both busses, but only be
talking to the helper on the system bus.
With polkit you currently request something over DBus from a helper and you
get a reply back. What you're describing you want is exactly that, except the
reply happens a file descriptor. (which as you hint is a native DBus type) Why
do you need to register a service in the ioslave?
INLINE COMMENTS
> sharefd.cpp:107
> +
> +bool FdReceiver::startListening(const std::string )
> +{
If this is on the client side, what stops any other (non authorised) client
listening to here?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D6709
To: chinmoyr, thiago, #frameworks
Cc: davidedmundson, elvisangelaccio, shortstheory
Minutes GSoC meeting
Present: me, chinmoy Minutes in the attachment. Cheers, Elvis2017-07-15 10:00:18 eangMeeting? 2017-07-15 10:00:39 chinmoy[m] i am here 2017-07-15 10:01:24 eanglet's start, as arnav said he wouldn't make it 2017-07-15 10:01:35 chinmoy[m] sure. 2017-07-15 10:02:19 chinmoy[m] this week i mostly created patches. 2017-07-15 10:03:01 chinmoy[m] however i found one serious flaw in my logic in showing the warning from ioslave 2017-07-15 10:03:47 chinmoy[m] in short with the available infrastructure showing warning at proper times is not just possible 2017-07-15 10:04:31 chinmoy[m] my previous patches just happen to work 2017-07-15 10:05:34 eangwhat's the problem? 2017-07-15 10:07:29 chinmoy[m] the warning from ioslave is only shown properly when a single folder is being deleted and it contains one or more read restricted file. 2017-07-15 10:08:33 chinmoy[m] with arnav's kio-stash and in copy operation the warning would have been shown as many times as there are read-restricted files 2017-07-15 10:11:12 eangI see, how are you planning to address this? 2017-07-15 10:13:48 chinmoy[m] In case you are interested, here's the cause. KIO::del and copy are high level jobs. they create number of subjobs according to the number of items, so, there are that many ioslaves as well. And there is no way for an ioslave to check for existence of its other siblings. 2017-07-15 10:14:46 chinmoy[m] Right now, I removed the code for warning from ioslave 2017-07-15 10:16:14 chinmoy[m] added a job flag PrivilegeExecution for application to use and a public method enablePrivilegeExecution which will be used from the job's side if the above flag is encountered. 2017-07-15 10:17:02 chinmoy[m] and those jobs which use this method will also show warning dialogs as well 2017-07-15 10:19:11 eangthe problem of checking whether other ioslaves are running could be solved with a kded plugin 2017-07-15 10:19:13 chinmoy[m] and when execAsPrivilegeUser is called a signal will be sent to job requessting if the method should be executed and it will be the responsibility of the job to send back a message containing "ElevatePrivilege" 2017-07-15 10:19:33 eang(in case you want to give it a shot) 2017-07-15 10:19:59 chinmoy[m] can you explain it a bit 2017-07-15 10:20:48 eangkded is always running, so an ioslave can query it any time 2017-07-15 10:21:10 eangthe plugin could have dbus methods such as "registerJob", "isJobRunning", etc. 2017-07-15 10:22:22 chinmoy[m] the jobs are scheduled synchronously (addSubJob) so i dont think it would serve the purpose 2017-07-15 10:23:04 chinmoy[m] the slaves will be destroyed as soon as the job finishes no? 2017-07-15 10:24:27 eangyes, but maybe it's possible to query kded between schedule time and deletion time? I don't know 2017-07-15 10:24:47 eanganyway, it would complicate things for sure 2017-07-15 10:25:18 chinmoy[m] definitely 2017-07-15 10:25:36 chinmoy[m] well give me few minutes I will update my patch 2017-07-15 10:26:14 chinmoy[m] I think with my current approach the loopholes David mentioned should be no more 2017-07-15 10:27:06 eangok sounds good 2017-07-15 10:28:44 eangchinmoy: anything else you wanted to discuss? 2017-07-15 10:31:36 chinmoy[m] there's one thing, what does BIC stands for? I think i didn't get it properly. 2017-07-15 10:32:05 eanghave you read https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B ? 2017-07-15 10:32:26 chinmoy[m] ok BCI 2017-07-15 10:33:42 chinmoy[m] I am slowly familiarising myself with abbreviations 2017-07-15 10:34:07 eangI'm actually not sure what BIC stands for 2017-07-15 10:34:07 chinmoy[m] so it is BIC after all 2017-07-15 10:34:23 eangthe important thing is you understand the concept 2017-07-15 10:34:45 eang(it could be Binary Interface Compatibility or something similar) 2017-07-15 10:35:04 chinmoy[m] whatever, now i understood 2017-07-15 10:35:44 eangmeeting over then? 2017-07-15 10:36:01 chinmoy[m] i think yes
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY Some methods in file ioslave, `FileProtocol::copy` and FileProtocol::put to be precise, use file descriptor of source and destination files. So performing any these operations as root user using kauth's helper requires the source or destination file to be opened inside the helper and sending the file descriptor back to ioslave using a suitable IPC mechanism. My patch does the task using unix local domain socket. In principal dbus can also be used. The sequence would be, registering service in ioslave, setting `euid` of the helper process and sending the file descriptor over user's session bus. I tried it but the code turned out messy. In the end it was somewhat a personal preference. There are certain things I would like to know regarding this patch, In this patch I used the abstract namespace. It will work with linux but I don't know about mac os or bsd. So what to use for them? In place of unix sockets, dbus can also be used. So shall i use it ? I am aware there are security issues with both the approaches but using which one of them is less riskier? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/sharefd.cpp src/ioslaves/file/sharefd.h To: chinmoyr, thiago, #frameworks Cc: elvisangelaccio, shortstheory
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
> On Lug. 15, 2017, 7:31 a.m., Simone Gaiarin wrote: > > Inviala! > > Luigi Toscano wrote: > You can't "ship it" you own review. "Ship it" is marked by other > reviewers who thinks that this should go in. > Do you mean to say that you don't have a developer account and someone > else should push this? I'm not very familiar with review board. Indeed I don't have a developer account. I thought that once approved by clicking ship it it would go through, but then I realized what "ship it" is for, so I was just waiting for someone to push it. So yes, I need someone with a developer account to push it please. - Simone --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/#review103453 --- On Lug. 12, 2017, 6:54 p.m., Simone Gaiarin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130180/ > --- > > (Updated Lug. 12, 2017, 6:54 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 359233 > https://bugs.kde.org/show_bug.cgi?id=359233 > > > Repository: kio > > > Description > --- > > The current "open with" dialog implementation does not follow the KDE > principle "Simple by default, powerful when needed" for the following reasons: > - The "run in terminal" and "keep terminal open" options are advanced options > and should not be exposed by default > - The primary goal of the dialog should be to select an application from the > app tree, running command is an advanced feature > > My patch changes the behavior as follow: > > - Put the two options in a KCollapsibleComboBox collapsed by default > - The user can expand it only if he needs to use a command line command > > > Implementation details: > - When the KCollapsibleComboBox is clicked it is expanded upward keeping the > dialog size fixed and compressing the treeview, I'm not sure this is the best > approach, but to make it expand downwards we need to fix the size of the > dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be > desiderable. Maybe there is a way to keep the dialog resizable and expand the > combobox downwards, but I couldn't find it. > - I've increased the vertical size of the dialog (which I think it was too > small) also to accomodate the upward expansion which otherwise would make the > app tree almost disappear > > > Relevant discussions: > https://bugs.kde.org/show_bug.cgi?id=359233 > https://forum.kde.org/viewtopic.php?f=285=131014 > > > Diffs > - > > src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 > > Diff: https://git.reviewboard.kde.org/r/130180/diff/ > > > Testing > --- > > > Thanks, > > Simone Gaiarin > >
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
> On Lug. 15, 2017, 9:31 a.m., Simone Gaiarin wrote: > > Inviala! You can't "ship it" you own review. "Ship it" is marked by other reviewers who thinks that this should go in. Do you mean to say that you don't have a developer account and someone else should push this? - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/#review103453 --- On Lug. 12, 2017, 8:54 p.m., Simone Gaiarin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130180/ > --- > > (Updated Lug. 12, 2017, 8:54 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 359233 > https://bugs.kde.org/show_bug.cgi?id=359233 > > > Repository: kio > > > Description > --- > > The current "open with" dialog implementation does not follow the KDE > principle "Simple by default, powerful when needed" for the following reasons: > - The "run in terminal" and "keep terminal open" options are advanced options > and should not be exposed by default > - The primary goal of the dialog should be to select an application from the > app tree, running command is an advanced feature > > My patch changes the behavior as follow: > > - Put the two options in a KCollapsibleComboBox collapsed by default > - The user can expand it only if he needs to use a command line command > > > Implementation details: > - When the KCollapsibleComboBox is clicked it is expanded upward keeping the > dialog size fixed and compressing the treeview, I'm not sure this is the best > approach, but to make it expand downwards we need to fix the size of the > dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be > desiderable. Maybe there is a way to keep the dialog resizable and expand the > combobox downwards, but I couldn't find it. > - I've increased the vertical size of the dialog (which I think it was too > small) also to accomodate the upward expansion which otherwise would make the > app tree almost disappear > > > Relevant discussions: > https://bugs.kde.org/show_bug.cgi?id=359233 > https://forum.kde.org/viewtopic.php?f=285=131014 > > > Diffs > - > > src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 > > Diff: https://git.reviewboard.kde.org/r/130180/diff/ > > > Testing > --- > > > Thanks, > > Simone Gaiarin > >
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/#review103453 --- Ship it! Inviala! - Simone Gaiarin On Lug. 12, 2017, 6:54 p.m., Simone Gaiarin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130180/ > --- > > (Updated Lug. 12, 2017, 6:54 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 359233 > https://bugs.kde.org/show_bug.cgi?id=359233 > > > Repository: kio > > > Description > --- > > The current "open with" dialog implementation does not follow the KDE > principle "Simple by default, powerful when needed" for the following reasons: > - The "run in terminal" and "keep terminal open" options are advanced options > and should not be exposed by default > - The primary goal of the dialog should be to select an application from the > app tree, running command is an advanced feature > > My patch changes the behavior as follow: > > - Put the two options in a KCollapsibleComboBox collapsed by default > - The user can expand it only if he needs to use a command line command > > > Implementation details: > - When the KCollapsibleComboBox is clicked it is expanded upward keeping the > dialog size fixed and compressing the treeview, I'm not sure this is the best > approach, but to make it expand downwards we need to fix the size of the > dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be > desiderable. Maybe there is a way to keep the dialog resizable and expand the > combobox downwards, but I couldn't find it. > - I've increased the vertical size of the dialog (which I think it was too > small) also to accomodate the upward expansion which otherwise would make the > app tree almost disappear > > > Relevant discussions: > https://bugs.kde.org/show_bug.cgi?id=359233 > https://forum.kde.org/viewtopic.php?f=285=131014 > > > Diffs > - > > src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 > > Diff: https://git.reviewboard.kde.org/r/130180/diff/ > > > Testing > --- > > > Thanks, > > Simone Gaiarin > >
Re: Review Request 130180: Make advanced options of "open with" dialog collabsible and hidden by default
> On Lug. 14, 2017, 8:24 p.m., David Faure wrote: > > Enlarging the dialog when showing a child widget can sometimes be done by > > activating the toplevel layout, but with a listview above I'm not sure it > > would work. > > Anyway, I like how it behaves with your patch, I find it quite nice > > actually, to make things fit within the window rather than forcefully > > resizing the window. Ok. It seems fine to me as well. - Simone --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130180/#review103451 --- On Lug. 12, 2017, 6:54 p.m., Simone Gaiarin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130180/ > --- > > (Updated Lug. 12, 2017, 6:54 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 359233 > https://bugs.kde.org/show_bug.cgi?id=359233 > > > Repository: kio > > > Description > --- > > The current "open with" dialog implementation does not follow the KDE > principle "Simple by default, powerful when needed" for the following reasons: > - The "run in terminal" and "keep terminal open" options are advanced options > and should not be exposed by default > - The primary goal of the dialog should be to select an application from the > app tree, running command is an advanced feature > > My patch changes the behavior as follow: > > - Put the two options in a KCollapsibleComboBox collapsed by default > - The user can expand it only if he needs to use a command line command > > > Implementation details: > - When the KCollapsibleComboBox is clicked it is expanded upward keeping the > dialog size fixed and compressing the treeview, I'm not sure this is the best > approach, but to make it expand downwards we need to fix the size of the > dialog with setSizeConstraint(QLayout::SetFixedSize); which may not be > desiderable. Maybe there is a way to keep the dialog resizable and expand the > combobox downwards, but I couldn't find it. > - I've increased the vertical size of the dialog (which I think it was too > small) also to accomodate the upward expansion which otherwise would make the > app tree almost disappear > > > Relevant discussions: > https://bugs.kde.org/show_bug.cgi?id=359233 > https://forum.kde.org/viewtopic.php?f=285=131014 > > > Diffs > - > > src/widgets/kopenwithdialog.cpp b28c5b05186bc77413524c99ef61b7967b0ec8b9 > > Diff: https://git.reviewboard.kde.org/r/130180/diff/ > > > Testing > --- > > > Thanks, > > Simone Gaiarin > >
