Multipaste - Fixed #2328

AdamDanischewski · 2019-09-27T16:59:21Z

Changed sci_wrapper.c::sci_set_multipaste() from hardcoded value SC_MULTIPASTE_EACH to function parameter val.

…E_EACH - fixes the paste only one line when a vertical selection is selected.

Reverted to Scintilla default ship.

…into multipaste

Fixed spacing.

Copy paste error fix.

Fixed hardcoding the value set from SC_MULTIPASTE_EACH to the function parameter.

codebrainz · 2019-09-28T00:48:30Z

This looks like the same topic/changes as #2325 and #2327. Please don't make multiple PRs for the same thing, just update the original one (ie. #2325). If you're having problems with Git, which is completely understandable as it has a steep learning curve, just ask about it in the comments or on the mailing list or IRC or wherever.

AdamDanischewski · 2019-09-28T18:32:46Z

@codebrainz They are the same. #2325 was approved but had a subsequent update. I'll keep it on the same PR in the future. For now I'll leave this open since there is no point to going back to #2325.

codebrainz · 2019-09-28T20:16:48Z

Ideally the latter two PRs would be closed and the original updated since it has comments and reviews on it.

AdamDanischewski · 2019-09-28T23:59:42Z

This also has comments, it still needs to be re-reviewed. No?

elextr · 2019-09-29T04:39:15Z

@AdamDanischewski Geany operates on tab == 4 not tab == 8, please set Geany correctly and undo your alignment changes to sciwrapper.h

elextr · 2019-09-29T04:39:42Z

And FGS don't make another PR do it on this one 😁

AdamDanischewski · 2019-09-29T12:10:24Z

I tried to be nice and clean it up, I really don't have time for this project. Go ahead and review the code and put it through or let me know if there is an actual problem. The team here seems very ungrateful for an extra helping hand at fixing a problem that's making Geany look bad.

@elextr You can open up your own PR for the cosmetics. ;-)

b4n

With a quick test, I agree that changing the behavior makes sense and is mostly compatible.
For the implementation part, I think this should be all what's needed (no need for a new Scintilla wrapper for something only used once in editor.c, and the function shouldn't be exposed in the headers anyway):

From 9b503708203a4986736da33d882f695dee6119c0 Mon Sep 17 00:00:00 2001
From: Colomban Wendling <ban@herbesfolles.org>
Date: Mon, 30 Sep 2019 20:09:12 +0200
Subject: [PATCH] Enable multi-selection clipboard paste

When pasting a single-line clipboard content inside multiple or
rectangular selections, paste it in each selection rather than only
in the main one.

This changes behavior, but the old behavior is easily reproduced by
canceling a multiple selection when a single paste is desired, whereas
it was not possible to obtain multiple pastes before.

diff --git a/src/editor.c b/src/editor.c
index df25c8808..b02705e36 100644
--- a/src/editor.c
+++ b/src/editor.c
@@ -4940,6 +4940,10 @@ static ScintillaObject *create_new_sci(GeanyEditor *editor)
 #endif
 	SSM(sci, SCI_SETRECTANGULARSELECTIONMODIFIER, rectangular_selection_modifier, 0);

+	/* With multiple selections, paste in them all (doesn't affect pasting
+	 * multi-line data in a rectangular selection) */
+	SSM(sci, SCI_SETMULTIPASTE, SC_MULTIPASTE_EACH, 0);
+
 	/* virtual space */
 	SSM(sci, SCI_SETVIRTUALSPACEOPTIONS, editor_prefs.show_virtual_space, 0);

I'd be happy to get this through, but I won't open a new PR just yet (hehe) and I can't push here. Of course I can create a new PR if @elextr's happy with it ;)

src/editor.c

b4n · 2019-09-30T18:22:49Z

src/sciwrappers.c

+ /* Set multi paste setting (at 3104 scintilla defaults to SC_MULTIPASTE_ONCE) */ 
+void sci_set_multipaste(ScintillaObject *sci, gint mpval) { 
+	SSM(sci, SCI_SETMULTIPASTE, mpval, 0); 
+}


We don't actually need a new wrapper for just one call, especially not when that call is in editor.c.

The reason I did this is because @elextr suggested that it should be an option. I agree that it should be the only behavior.

Related discussion can be found in #2317 and in #850

b4n · 2019-09-30T18:23:34Z

src/sciwrappers.h

-void 				sci_set_text				(ScintillaObject *sci,  const gchar *text);
-gboolean			sci_has_selection			(ScintillaObject *sci);
-void 				sci_end_undo_action			(ScintillaObject *sci);
+void                            sci_set_multipaste              (ScintillaObject *sci, gint mpval);


This should be in the GEANY_PRIVATE section below. Only plugin API functions should be outside of it.

Again the reason I did this is because @elextr suggested that it should be an option.

b4n · 2019-09-30T18:26:14Z

src/sciwrappers.h

@@ -56,162 +57,162 @@ void 				sci_set_current_position	(ScintillaObject *sci, gint position, gboolean

 gint				sci_get_selection_start		(ScintillaObject *sci);
 gint				sci_get_selection_end		(ScintillaObject *sci);
-void 				sci_replace_sel				(ScintillaObject *sci, const gchar *text);
+void 				sci_replace_sel			(ScintillaObject *sci, const gchar *text);


Please don't rewrite unrelated indentation, even if it was incorrect (and here it's not, Geany uses a tab width of 4, which aligns well). Changes like this make it a lot harder to read the actual, meaningful changes, and should be made in a specific commit if at all.

To be fair, aligning stuff like this (GNU/GTK+-style) is a terrible idea, especially when tabs are used. Not sure why this one file in Geany uses this alignment style.

Yeah, but to return it to its original state only needs git checkout src/sciwrappers.h. There was so much noise I didn't even notice that there was an actual change. @b4n did better.

The indentation is horrid in this file - I tried to be nice, it won't happen again.

The indentation is horrid in this file

👍

At some point I'll probably make a PR to reformat this header to follow Geany's coding style.

SC_MULTIPASTE_EACH is no longer an option.

Setting SC_MULTIPASTE_EACH is defaulted and therefore no longer exposed.

Setting SC_MULTIPASTE_EACH has been made the default.

AdamDanischewski · 2019-10-01T14:40:39Z

Updated - SC_MULTIPASTE_EACH is now set in geany/src/editor.c::editor_create_widget() without any wrappers since this is now the default.

b4n · 2019-10-01T14:52:42Z

Better. IMO we should just use my version that has all the small things I'd like to see changed here (comment, location of the change (which is my bad because of an incorrect comment, sorry), no leftover unrelated whitespace changes, clearer commit message, no leftover history), but basically I'm now happy with the spirit of the changes here.

AdamDanischewski · 2019-10-01T15:01:47Z

@b4n It sounds like you're not really interested in crediting me for fixing this - does this mean I won't be added to the list of contributors?

…direct scintilla settings are.

AdamDanischewski · 2019-10-01T15:52:04Z

Don't worry guys, all fixed. =)

b4n · 2019-10-01T18:44:06Z

@b4n It sounds like you're not really interested in crediting me for fixing this - does this mean I won't be added to the list of contributors?

To be blunt, I really don't care who's credited here :) But I'll happily credit you, my example was not trying to draw it away from you.

AdamDanischewski

The change request from b4n is already implemented.

src/editor.c

[Adam Danischewski] and others added 7 commits September 26, 2019 22:25

Changed the scintilla default from SC_MULTIPASTE_ONCE to SC_MULTIPAST…

3a1df4e

…E_EACH - fixes the paste only one line when a vertical selection is selected.

Update Editor.cxx

5a12780

Reverted to Scintilla default ship.

Moved scintilla SC_MULTIPASTE_EACH default setting to Geany.

f4672f1

Merge branch 'multipaste' of https://github.com/AdamDanischewski/geany …

a33ec29

…into multipaste

Update sciwrappers.h

f76cf9d

Fixed spacing.

Update Editor.cxx

c0c72bf

Copy paste error fix.

Update sciwrappers.c

63d3a9b

Fixed hardcoding the value set from SC_MULTIPASTE_EACH to the function parameter.

b4n requested changes Sep 30, 2019

View reviewed changes

AdamDanischewski added 3 commits October 1, 2019 10:31

SC_MULTIPASTE_EACH is default

f22fc13

SC_MULTIPASTE_EACH is no longer an option.

Setting SC_MULTIPASTE_EACH no longer exposed

9211428

Setting SC_MULTIPASTE_EACH is defaulted and therefore no longer exposed.

Setting SC_MULTIPASTE_EACH is no longer exposed

b78159d

Setting SC_MULTIPASTE_EACH has been made the default.

[Adam Danischewski] added 2 commits October 1, 2019 11:46

Reverted - a blank line at the end was missing

a1a185e

Moved SC_MULTIPASTE_EACH setting to create_new_sci() where the other …

79923d2

…direct scintilla settings are.

elextr mentioned this pull request Oct 2, 2019

Enhancement Request - Multiselect Multipaste Option #850

Open

AdamDanischewski commented Oct 6, 2019

View reviewed changes

src/editor.c Outdated Show resolved Hide resolved

AdamDanischewski requested a review from b4n October 6, 2019 01:07

AdamDanischewski changed the title ~~Multipaste~~ Multipaste - Fixed Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipaste - Fixed #2328

Multipaste - Fixed #2328

AdamDanischewski commented Sep 27, 2019

codebrainz commented Sep 28, 2019

AdamDanischewski commented Sep 28, 2019

codebrainz commented Sep 28, 2019

AdamDanischewski commented Sep 28, 2019

elextr commented Sep 29, 2019

elextr commented Sep 29, 2019

AdamDanischewski commented Sep 29, 2019

b4n left a comment

b4n Sep 30, 2019

AdamDanischewski Oct 1, 2019

Akronix Oct 2, 2019

b4n Sep 30, 2019

AdamDanischewski Oct 1, 2019

b4n Sep 30, 2019

codebrainz Sep 30, 2019

elextr Oct 1, 2019

AdamDanischewski Oct 1, 2019

codebrainz Oct 2, 2019

AdamDanischewski commented Oct 1, 2019

b4n commented Oct 1, 2019

AdamDanischewski commented Oct 1, 2019

AdamDanischewski commented Oct 1, 2019

b4n commented Oct 1, 2019

AdamDanischewski left a comment

Multipaste - Fixed #2328

Are you sure you want to change the base?

Multipaste - Fixed #2328

Conversation

AdamDanischewski commented Sep 27, 2019

codebrainz commented Sep 28, 2019

AdamDanischewski commented Sep 28, 2019

codebrainz commented Sep 28, 2019

AdamDanischewski commented Sep 28, 2019

elextr commented Sep 29, 2019

elextr commented Sep 29, 2019

AdamDanischewski commented Sep 29, 2019

b4n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamDanischewski commented Oct 1, 2019

b4n commented Oct 1, 2019

AdamDanischewski commented Oct 1, 2019

AdamDanischewski commented Oct 1, 2019

b4n commented Oct 1, 2019

AdamDanischewski left a comment

Choose a reason for hiding this comment