Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, Oct 19, 2017 at 12:47:41PM +0800, 黄建忠 wrote:
> When using ctrl-p to load uri from clipboard, it's better to strip the
> leading whitespace.
>
> For example, to select/copy a uri from text in terminal and paste to
> surf, currently it need to be very careful not to include any whitespace
> before the uri.
>
> It's easy for keyboard selection, but for mouse selection, precise
> positioning is a little bit difficult.
>
>
> patch as below:
>
> diff -Nur surf/surf.c surfn/surf.c
> --- surf/surf.c 2017-10-17 13:58:00.636699137 +0800
> +++ surfn/surf.c 2017-10-17 13:58:29.440798516 +0800
> @@ -1707,7 +1707,8 @@
> void
> pasteuri(GtkClipboard *clipboard, const char *text, gpointer d)
> {
> - Arg a = {.v = text };
> + char *trimed = g_strstrip(g_strdup(text));
> + Arg a = {.v = trimed };
> if (text)
> loaduri((Client *) d, &a);
> }
>
>
> --
> Huang JianZhong
>
>
Hey,
Doesn't this leak memory?
(NOTE: for single-use programs it is ok to not free memory at the end).
--
Kind regards,
Hiltjo
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
在 2017年10月19日 16:00, Hiltjo Posthuma 写道:
> On Thu, Oct 19, 2017 at 12:47:41PM +0800, 黄建忠 wrote:
>> When using ctrl-p to load uri from clipboard, it's better to strip the
>> leading whitespace.
>>
>> For example, to select/copy a uri from text in terminal and paste to
>> surf, currently it need to be very careful not to include any whitespace
>> before the uri.
>>
>> It's easy for keyboard selection, but for mouse selection, precise
>> positioning is a little bit difficult.
>>
>>
>> patch as below:
>>
>> diff -Nur surf/surf.c surfn/surf.c
>> --- surf/surf.c 2017-10-17 13:58:00.636699137 +0800
>> +++ surfn/surf.c 2017-10-17 13:58:29.440798516 +0800
>> @@ -1707,7 +1707,8 @@
>> void
>> pasteuri(GtkClipboard *clipboard, const char *text, gpointer d)
>> {
>> - Arg a = {.v = text };
>> + char *trimed = g_strstrip(g_strdup(text));
>> + Arg a = {.v = trimed };
>> if (text)
>> loaduri((Client *) d, &a);
>> }
>>
>>
>> --
>> Huang JianZhong
>>
>>
> Hey,
>
> Doesn't this leak memory?
>
> (NOTE: for single-use programs it is ok to not free memory at the end).
>
Tks, g_strdup need g_free, g_strstrip will not allocate new memory.
Maybe it should changed like this?
if (text) {
char *atext = g_strdup(text);
char *trimed = g_strstrip(atext);
Arg a = {.v = trimed };
loaduri((Client *) d, &a);
g_free(atext);
}
--
Huang JianZhong
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
> - Arg a = {.v = text };
> + char *trimed = g_strstrip(g_strdup(text));
> + Arg a = {.v = trimed };
Doesn't this leak memory via strdup on every paste? Or does Gobject do
some automagic ref counting or whatnot?
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, 19 Oct 2017, Kamil Cholewiński wrote:
>> - Arg a = {.v = text };
>> + char *trimed = g_strstrip(g_strdup(text));
>> + Arg a = {.v = trimed };
>
> Doesn't this leak memory via strdup on every paste? Or does Gobject do
> some automagic ref counting or whatnot?
(Ignore, didn't see the other reply, my email client sometimes fails to
group threads.)
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
Hello Jianzhong,
> When using ctrl-p to load uri from clipboard, it's better to strip the
> leading whitespace.
I'm not sure it is.
> For example, to select/copy a uri from text in terminal and paste to
> surf, currently it need to be very careful not to include any
> whitespace before the uri.
Yes, yo do.
> It's easy for keyboard selection, but for mouse selection, precise
> positioning is a little bit difficult.
I'd suggest you manage this outside of surf, either by training your
mouse skills, or by stripping those leading whitespaces in your
selecting application.
> patch as below:
>
> diff -Nur surf/surf.c surfn/surf.c
> --- surf/surf.c 2017-10-17 13:58:00.636699137 +0800
> +++ surfn/surf.c 2017-10-17 13:58:29.440798516 +0800
> @@ -1707,7 +1707,8 @@
> void
> pasteuri(GtkClipboard *clipboard, const char *text, gpointer d)
> {
> - Arg a = {.v = text };
> + char *trimed = g_strstrip(g_strdup(text));
> + Arg a = {.v = trimed };
> if (text)
> loaduri((Client *) d, &a);
> }
As other stated, you're allocating a new string there whithout ever
releasing it which is wrong.
I'd suggest a simpler approach like this :
diff --git a/surf.c b/surf.c
index 0f8b9c9..8a40a3b 100644
--- a/surf.c
+++ b/surf.c
@@ -1685,9 +1685,14 @@ destroywin(GtkWidget* w, Client *c)
void
pasteuri(GtkClipboard *clipboard, const char *text, gpointer d)
{
- Arg a = {.v = text };
- if (text)
+ Arg a;
+
+ if (text) {
+ for (; *text && (*text == ' ' || *text == '\t'); ++text)
+ ;
+ a.v = text;
loaduri((Client *) d, &a);
+ }
}
void
---
I'm open to discuss this further though if more people complain about
it, thanks for your interest in surf!
-- Quentin
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, 19 Oct 2017 11:11:13 +0200
Kamil Cholewiński wrote:
Dear Kamil,
> > - Arg a = {.v = text };
> > + char *trimed = g_strstrip(g_strdup(text));
> > + Arg a = {.v = trimed };
>
> Doesn't this leak memory via strdup on every paste? Or does Gobject do
> some automagic ref counting or whatnot?
sorry for the GitHub-links, but I investigated this further and found
no better public source of the GNOME glib.
g_strdup() is defined here[0], which itself makes a call to g_new()[1],
which itself is an alias for the internal macro _G_NEW with a parameter
to call g_malloc_n()[2], which makes an overflow check (fair enough, it
corresponds to reallocarray()), and then calls g_malloc()[3].
As a small fun part, there's also a function g_malloc0_n() just below
g_malloc_n() which does exactly the same (the code is identical).
I think they created g_malloc0_n() to be a "safe" interface while
keeping g_malloc_n() for "performance" reasons, but then later on
noticed that it might be smart to do the check anyway, having two
identical functions and bloating the API unnecessarily.
Alright, we are now at g_malloc(), and as we can see in line 94 it's
just malloc() with some GNOME-vomit around it.
TL;DR: g_strdup() == strdup()
Reading this code is a real pain. It's like diving into a fractal,
where every function decomposes into even smaller parts, and on each
level, everything looks the same.
With best regards
Laslo Hunhold
[0]: https://github.com/GNOME/glib/blob/master/glib/gstrfuncs.c#L344
[1]: https://github.com/GNOME/glib/blob/master/glib/gmem.h#L245
[2]: https://github.com/GNOME/glib/blob/master/glib/gmem.c#L310
[3]: https://github.com/GNOME/glib/blob/master/glib/gmem.c#L78
--
Laslo Hunhold
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
Nice digging, thanks Laslo. > TL;DR: g_strdup() == strdup() And this is why the world needs suckless. Who cares if it's open source, if the code is incomprehensible? <3,K.
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, Oct 19, 2017 at 3:43 PM, Kamil Cholewiński wrote: > Nice digging, thanks Laslo. > >> TL;DR: g_strdup() == strdup() > > And this is why the world needs suckless. > > Who cares if it's open source, if the code is incomprehensible? I care, because if it wasn't open source you couldn't see how shitty the code is :P Cheers, Silvan
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
Hi Laslo, On 19 October 2017 at 15:17, Laslo Hunhold wrote: [..] > Alright, we are now at g_malloc(), and as we can see in line 94 it's > just malloc() with some GNOME-vomit around it. > > TL;DR: g_strdup() == strdup() > > Reading this code is a real pain. It's like diving into a fractal, > where every function decomposes into even smaller parts, and on each > level, everything looks the same. Nice investigation. And it proves once again, that GNU's not Unix and that those guys must be on drugs and/or alcohol/medication permanently. BR, Anselm
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, 19 Oct 2017 15:17:43 +0200 Laslo Hunhold wrote: > As a small fun part, there's also a function g_malloc0_n() just below > g_malloc_n() which does exactly the same (the code is identical). I > think they created g_malloc0_n() to be a "safe" interface while > keeping g_malloc_n() for "performance" reasons, but then later on > noticed that it might be smart to do the check anyway, having two > identical functions and bloating the API unnecessarily. Sorry, I need to correct myself here. I misread the code to be honest, and g_malloc0_n() is in fact like a "calloc". I hope this can be forgiven given the nature of the code. The other points still stand though of course. :) With best regards Laslo -- Laslo Hunhold
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
Please folks, don't start yet another endless diverging thread about how *thing* is ugly and all. There's no need to write so much about what g_strdup() is, seriously, were you born yesterday? Keep the threads about what they are initially, start your own apart if you want to diverge from it. There's enough pollution on the list. > On Thu, 19 Oct 2017 15:17:43 +0200 > Laslo Hunhold wrote: > > > As a small fun part, there's also a function g_malloc0_n() just > > below g_malloc_n() which does exactly the same (the code is > > identical). I think they created g_malloc0_n() to be a "safe" > > interface while keeping g_malloc_n() for "performance" reasons, but > > then later on noticed that it might be smart to do the check > > anyway, having two identical functions and bloating the API > > unnecessarily. > > Sorry, I need to correct myself here. I misread the code to be honest, > and g_malloc0_n() is in fact like a "calloc". I hope this can be > forgiven given the nature of the code. > The other points still stand though of course. :) > > With best regards > > Laslo >
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, 19 Oct 2017 17:36:15 +0200 Quentin Rameau wrote: Hey Quentin, > There's enough pollution on the list. that's true, but it was a genuine question both Kamil and I had, and it served the question. If GNOME for some reason had an internal ref-counter for objects allocated with g_strdup(), we might have looked at the patch a bit differently. But, as expected, it's just more bloat from the GNU-crowd. It is important to factor out harmful concepts like these. Everyone who read the thread will now think twice about using a GNOME function when he's confronted with it. With best regards Laslo -- Laslo Hunhold
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
> On Thu, 19 Oct 2017 17:36:15 +0200 > Quentin Rameau wrote: > > Hey Quentin, > > > There's enough pollution on the list. > > that's true, but it was a genuine question both Kamil and I had, and > it served the question. If GNOME for some reason had an internal > ref-counter for objects allocated with g_strdup(), we might have > looked at the patch a bit differently. But, as expected, it's just > more bloat from the GNU-crowd. > It is important to factor out harmful concepts like these. Everyone > who read the thread will now think twice about using a GNOME function > when he's confronted with it. Please stop trying to justify yourself by stating the obvious, enough already.
Re: [dev] [surf] [patch] strip uri leading whitespace when ctrl-p
On Thu, Oct 19, 2017 at 12:19:13PM +0200, Quentin Rameau wrote:
> Hello Jianzhong,
> > It's easy for keyboard selection, but for mouse selection, precise
> > positioning is a little bit difficult.
>
> I'd suggest you manage this outside of surf, either by training your
> mouse skills, or by stripping those leading whitespaces in your
> selecting application.
>
No. For one thing, if he actually did this, I foresee a patch in the
near future, this time for st, to strip white space off of the
selection, and it will be rejected by you or someone like you, saying
"this should be handled by the target application". Sending people from
Pontius to Pilate like that is not very nice.
For two, in this case the target application is the one that knows
something about the text to be pasted, namely, that it's a URI. Since
those never start with white space, surf is actually in the better
position to perform any string manipulation, that if some other
application had to try and guess at the intent of the user.
> I'd suggest a simpler approach like this :
>
> diff --git a/surf.c b/surf.c
> index 0f8b9c9..8a40a3b 100644
> --- a/surf.c
> +++ b/surf.c
> @@ -1685,9 +1685,14 @@ destroywin(GtkWidget* w, Client *c)
> void
> pasteuri(GtkClipboard *clipboard, const char *text, gpointer d)
> {
> - Arg a = {.v = text };
> - if (text)
> + Arg a;
> +
> + if (text) {
> + for (; *text && (*text == ' ' || *text == '\t'); ++text)
> + ;
> + a.v = text;
> loaduri((Client *) d, &a);
> + }
> }
>
Thanks. I already thought the simple loop solution was never coming up.
Ciao,
Markus
