Added the "save selection/cut selection to file" to Edit menu
AbandonedPublic

Authored by lexdem on Oct 14 2017, 4:45 PM.

Details

Reviewers
None
Group Reviewers
Kate
Summary

This is the patch for the following bug/feature: 377538

Test Plan
  • Check please for memory leaks. I'm still new for this. This patch (addon) doesn't have a destructor. I don't know if this is necessary
  • I have checked manually. Typed some text and pressed the buttons to perform copy/delete actions. Still, I don't know if that will work with any non-Unicode symbols

Diff Detail

Repository
R40 Kate
Lint
Lint Skipped
Unit
Unit Tests Skipped
lexdem created this revision.Oct 14 2017, 4:45 PM
lexdem updated this revision to Diff 20750.Oct 14 2017, 4:49 PM

Removed useless commentary

Thanks for the patch! Can you change the formatting here on Phabricator a bit?

  • Title should be a short description of the actual feature
  • Don't need FEATURE
  • The bug number goes in the Summary section, like this: "BUG: 377538"
  • Add some details about your testing to the Test Plan section
  • Add some screenshots of the feature! This is always welcome for attracting reviewers. :)
lexdem retitled this revision from **FEATURE**: 377538 to Added the "save selection/cut selection to file" to Edit menu.Oct 14 2017, 5:41 PM
lexdem edited the summary of this revision. (Show Details)
lexdem edited the test plan for this revision. (Show Details)

Thanks for the patch! Can you change the formatting here on Phabricator a bit?

  • Title should be a short description of the actual feature
  • Don't need FEATURE
  • The bug number goes in the Summary section, like this: "BUG: 377538"
  • Add some details about your testing to the Test Plan section
  • Add some screenshots of the feature! This is always welcome for attracting reviewers. :)

Changed. Sorry, couldn't take screenshot from "Edit" menu in Spectacle. IDK why, but it just don't capture the active popups :(

! In D8300#155426, @lexdem wrote:
Changed. Sorry, couldn't take screenshot from "Edit" menu in Spectacle. IDK why, but it just don't capture the active popups :(

Yeah that's an X11 bug/limitation. The workaround is to open Spectacle, set it to take a screenshot in a few seconds, and then use that time to open the menu you want to screenshot.

There are a few typos in your Test Plan:

Don;t know, if it necessary -> I don't know if this is necessary

lexdem edited the summary of this revision. (Show Details)Oct 14 2017, 6:02 PM
lexdem edited the test plan for this revision. (Show Details)

! In D8300#155426, @lexdem wrote:
Changed. Sorry, couldn't take screenshot from "Edit" menu in Spectacle. IDK why, but it just don't capture the active popups :(

Yeah that's an X11 bug/limitation. The workaround is to open Spectacle, set it to take a screenshot in a few seconds, and then use that time to open the menu you want to screenshot.

There are a few typos in your Test Plan:

Don;t know, if it necessary -> I don't know if this is necessary

Yeay, that worked :) With the "Rectangular region". Anyway, thanks, and here's the new edits

Capitalization and wording is a bit off. How about this:

Save selection to a file -> New File from Selection

Also, do we really need the Cut + new file version? Is the copy + new file version enough?

lexdem updated this revision to Diff 20756.Oct 14 2017, 7:10 PM

Changed the strings

Also, do we really need the Cut + new file version? Is the copy + new file version enough?

In that bug report there is no mention about the versions, but I think, it is pretty neat to just press one button without the necessity to press the Backspace key again. You know, perhaps, someone just wants to cut the code and paste to another one. So if there only will be "save the selected", person will need to have an additional press on backspace button. Thinking about productivity, hah :)

lexdem updated this revision to Diff 20757.Oct 14 2017, 7:14 PM

Removed unused commentary in headers.

anthonyfieroni added inline comments.
addons/save_selection_to_file/CMakeLists.txt
25

Add new line

addons/save_selection_to_file/saveselection.h
20

Use C++11 feature

const QList<QVariant> & = {}
32

New line and remove indent.

lexdem updated this revision to Diff 20760.Oct 14 2017, 7:57 PM

Added new lines to the end of files. Removed indentation

lexdem updated this revision to Diff 20976.Oct 19 2017, 2:09 PM
lexdem marked 3 inline comments as done.

One more i18n for better UX.

Hello. Have anyone found some errors? This patch exists over a week. I'm not saying, that I pressure anyone, I just afraid, if the version will increase in kateui.rc , the build could not pass. Could it be a problem? I mean, if there will be a patch which will increment the the number to 83, can this diff be passed?

brauch added a subscriber: brauch.Oct 22 2017, 9:49 AM

Hmm, I'm not sure -- this is quite a bit of code for a feature I personally wouldn't know how to make use of. Yes, I sometimes want to perform this action, but really, I just press Ctrl+C, Ctr+N, Ctrl+V and I have achieved the same thing -- and it's much easier to remember than another shortcut for specifically this ...

The implementation itself looks fine, I'm just unsure whether this necessitates a whole new plugin.

Hmm, I'm not sure -- this is quite a bit of code for a feature I personally wouldn't know how to make use of. Yes, I sometimes want to perform this action, but really, I just press Ctrl+C, Ctr+N, Ctrl+V and I have achieved the same thing -- and it's much easier to remember than another shortcut for specifically this ...

The implementation itself looks fine, I'm just unsure whether this necessitates a whole new plugin.

Thanks for response.
You mean, it should be added to core, not via the plugin?
As for me, it is useful, that you can turn it on/off. Why not :) Moreover, I think, that the best way is just to provide just the base of kate and the downloadable parts, which can increase user productivity. Kinda like extensions in VS Code.
Yet, it is another tool for shorten the keystrokes - you mentioned three, and this "save_selection" can be configured to new keystroke. Which will be just one :) But I cannot specify which one. Anyway, I do not insist. This can be a pretty neat feature, but it should be optional. Therefore, I created it as a plugin

Yeah, the problem with kate plugins is that we have no way to distribute them. We don't have infrastructure for distributing binary plugins, and we don't really support script plugins, and nobody installs your plugin if you put it on github with compile instructions. I'm not sure what that means for this case, I'll leave that decision to the maintainers I think.

sars added a subscriber: sars.Oct 22 2017, 7:28 PM

Hi,

Are you aware that dragging a selection to anywhere in kate, except the document view, will create a new file with the selection. But that requires mouse action....

In D8300#158379, @sars wrote:

Hi,

Are you aware that dragging a selection to anywhere in kate, except the document view, will create a new file with the selection. But that requires mouse action....

No, i didn't know that yet. Thanks for the hint :)
Anyway, if you think, this is useless patch, don't merge it. But the feature report is existing, so either it is needed to be closed, or proposed the alternative. But again, it is optional, and not enabled by default. Maybe someone can find this plugin helpful
Thanks for your response :)

Hi, I am not sure this feature is that important to have.

If I read the feature request you linked correctly, what is implemented here is actually not that what is wanted there.

https://bugs.kde.org/show_bug.cgi?id=377538

looks more like one wants a fast way to create a snippet library.

Perhaps this would be more interesting as an snippet plugin extension to easily create new snippets for e.g. copyright headers
or default class declarations?

Hi, I am not sure this feature is that important to have.

If I read the feature request you linked correctly, what is implemented here is actually not that what is wanted there.

https://bugs.kde.org/show_bug.cgi?id=377538

looks more like one wants a fast way to create a snippet library.

Perhaps this would be more interesting as an snippet plugin extension to easily create new snippets for e.g. copyright headers
or default class declarations?

Hello, could you please clarify a little bit more about that snippets tool? Because there exists already some kind of that. https://docs.kde.org/trunk5/en/applications/kate/kate-application-plugin-snippets.html
Is that needs to be user-specified directory, as stated in feature request? Here he says that it should be a some directory, like /home/porton/t.
What if it will be like a plugin, where user specifies directory. Then, based on the extension(like .cpp), it creates the file with same name, but with .h header and contains all that definitions with copyright headers?
It sounds too specific to one language, but maybe, it can be extended further.. Oh, and then it won't be at all junior-bug :) Or I overcomplicate this request and it just should be a directory, configured by user, and here should be extracted lines with different extension?

In the report, the user wanted some configurable directory.
I don't think this is something more than a few people want and not worth the effort.
The snippet tools stores the snippets in a user local config (if you add some as a user).
This would imho make most sense for me, to have in the snippet plugin some easy way to generate new ones from context menu or such, but I am not sure if there is not already the UI for that, as I never use the snippets stuff.
Perhaps you could just try out, what the snippet plugin provides.

lexdem added a comment.EditedOct 27 2017, 1:16 PM

I don't think this is something more than a few people want and not worth the effort.

I think, you're right. Just checked the snippets built-in plugin. Not kinda in my taste, but anyway, there is already a plugin, and I should not reinvent the wheel.

the user wanted some configurable directory.

As for this, it is a neat feature, but it just don't make a lot of sense.

Well, then just close this differential please :)
Maybe we should specify in that bug report, that this will not be fixed and close that too?

rkflx added a subscriber: rkflx.Oct 28 2017, 9:45 PM

I think, you're right.

Good job accepting this advice (which I agree with), such is not always easy :)

Well, then just close this differential please :)

As you are the author of this Diff, Phab expects this to be your job. You could use the "Abandon" state in this case.

Maybe we should specify in that bug report, that this will not be fixed and close that too?

First you could tell the reporter why we prefer not to add a new menu item. Then you could ask if any of the methods mentioned here (snippets library, keyboard shortcut, @sars trick) would work for him (set bug state to NEEDSINFO), or what he is missing in those.

(Maybe some of the reporter's needs could even be fulfilled by Kate's scripting API? Or a symlink as a "configurable" directory?)

lexdem abandoned this revision.Oct 31 2017, 7:29 AM
In D8300#161302, @rkflx wrote:

Good job accepting this advice (which I agree with), such is not always easy :)

Thanks. I've understood my mistakes :)

In D8300#161302, @rkflx wrote:

You could use the "Abandon" state in this case.

Yep, just figured it out, that it is in the comments section :)

In D8300#161302, @rkflx wrote:

First you could tell the reporter why we prefer not to add a new menu item. Then you could ask if any of the methods mentioned here (snippets library, keyboard shortcut, @sars trick) would work for him (set bug state to NEEDSINFO), or what he is missing in those.

(Maybe some of the reporter's needs could even be fulfilled by Kate's scripting API? Or a symlink as a "configurable" directory?)

Yep, that's right. Ok, I will try to inform the reporter :) Thanks and see you in the next differential ;)