Selection in text_field

[Alia]

Here is basic idea of how it can be made

 

https://developer.berlios.de/patch/index.ph...p;group_id=1256

[ttlanhil]

I haven't tried it out yet, but I'm already excited

 

A few notes: it looks like you're only using window-manager and mouse-drag code for detecting selection.

It should be easy to expand this by allowing shift + direction (ctrl + or up/down/left/right/home/end) inside an editable text_area widget to move either the start or the end of the selection.

 

Also, it looks like you have a lot of the work done, how hard would it be to mark a start and end text_message line in console, so those can be selected as well?

 

I saw a calloc() call in there, are you actually allocating memory for your selected text, or just recording start/end positions (the patch itself probably doesn't seem to give enough context to be able to tell how your code interacts with the rest, so apologies if I'm asking a silly question)?

 

ed: I would look at this in more depth, since it's something I've tried before, but I'll leave it to Grum since he's been working with the notepad.

Edited June 14, 2007 by ttlanhil

[Grum]

Thanks, looks interesting, I'll take a better look when I have more time. I don't have substantial comments yet, but I did notice that part of the patch is indented with (4?) spaces. Please use tabs only for indentation (and set your editor to use 4 spaces per tab if you think 8 is too many).

 

EDIT: another quicky, in copying the messages, you break upon entering '\r' or '\n'. Hard newlines ('\n') should probably be copied into the buffer, soft newline ('\r') simply skipped over.

 

EDIT 2: please add functions descriptions to header files when you export new functions.

 

EDIT 3: there's already a define for IS_COLOR in asc.h

Edited June 14, 2007 by Grum

[Alia]

Thanks, looks interesting, I'll take a better look when I have more time. I don't have substantial comments yet, but I did notice that part of the patch is indented with (4?) spaces. Please use tabs only for indentation (and set your editor to use 4 spaces per tab if you think 8 is too many).

I use vim with tabstop=8, softtabstop=4. (It means if you view my code in some primitive editor which

supports only one type of tab, you should set tab to 8 spaces and indentation will appear to be 4 spaces)

However as my code is mixed with code from CVS it could look nasty.

To make things better I suggest to expand all tabs to spaces.

 

EDIT: another quicky, in copying the messages, you break upon entering '\r' or '\n'. Hard newlines ('\n') should probably be copied into the buffer, soft newline ('\r') simply skipped over.

Both produce new line.

 

EDIT 2: please add functions descriptions to header files when you export new functions.

Hmm, I'll try

 

EDIT 3: there's already a define for IS_COLOR in asc.h

I've seen it. But I don't know who have added it, and what is it expected to do. Therefore I'm

not going to use it, because someone can change it later and break my things.

There are MANY code duplications in client, so someone should clean it sometimes

[Grum]

EDIT: another quicky, in copying the messages, you break upon entering '\r' or '\n'. Hard newlines ('\n') should probably be copied into the buffer, soft newline ('\r') simply skipped over.

Both produce new line.

Yes, but you then skip the rest of the message. And '\r' should not give a newline, it should simply be ignored, since it's only there for wrapping.

 

EDIT 3: there's already a define for IS_COLOR in asc.h

I've seen it. But I don't know who have added it, and what is it expected to do. Therefore I'm

not going to use it, because someone can change it later and break my things.

There are MANY code duplications in client, so someone should clean it sometimes

Yes, and a good place to start would be not to introduce any extra duplication. asc.h contains text/char/string related functions, and the comment above the macro explicitly says

/*!
* Check if a character is a color character
*/

If anybody changes it (which is highly unlikely), then it's probably for a good reason.

 

EDIT: fix quote blocks

Edited June 14, 2007 by Grum

[Alia]

Yes, but you then skip the rest of the message. And '\r' should not give a newline, it should simply be ignored, since it's only there for wrapping.

 

Well, usually I think before I type. Look at draw_messages function: it starts new line when it hits

'\r', '\n' or '\0'. I've tried to make pasted text to look the same as it looked in client, so all these chars

should be converted to '\n' (at least for linux).

 

Anyway thank you for this discussion: i've found a bug

[Grum]

I know draw_messages inserts a newline upon '\r'.

 

The '\r' characters are inserted when you reach the end of the line on a text field. Ideally, when changing the width of the field, those characters are removed and possibly inserted at different positions so that the lines are filled out again. (I don't know if there are still any resizable text fields in the code, but at least the chat window used to do this). It's not a real new line, and IMNSHO it should not be inserted when copying the text to the clipboard. As an example, copy and paste text from the input field you're using to reply to this post to a different window. The newlines that you didn't type yourself are not reproduced in your pasted text.

 

And that still leaves the issue that you ignore everything in a message after a line break, be it a soft or hard one.

[Alia]

And that still leaves the issue that you ignore everything in a message after a line break, be it a soft or hard one.

 

I've said I've found a bug. It is even worse than only ignoring part of the text.

I'll fix it and update patch after I add keyboard selection for ttlanhil.

[Grum]

Ok, thanks

[Alia]

Patch updated:

+ Seletion with keyboard (only for editable text_fields)

+ DEL and BACKSPACE removes selected block if there is any selection.

+ Paste is possible to any editable text_field (not only into main line)

 

Notes (for someone who works on notepad)

1. Inestion and deletion of blocks should change number of lines and

current cursor line.

2. Selection cannot 'guess' color of the message in notepad, because there

are no color chars. Adding one in the beggining will fix it.

[Grum]

Thanks for updating the patch. I looked through it, except for most of the X-related stuff, for which I'd have to spend too much time flipping through man-pages Here are my comments:

• Indent with tabs instead of spaces.
  
• Add a description for append_char() in asc.h, and the functions added in paste.h
  
• Why is the check on mouse up/down events happening inside the window limits disabled?
  
• Please replace IS_COLOR_CHAR by IS_COLOR
  
• Would it be possible to reuse skip_message() in draw_messages() ?
  
• In text_field_get_selected_text(), the test
  

  if ((sm == em) && (sc > ec))
  

  
  shouldn't that be
  

  if ((sm == em) && (sc >= ec))
  

  
  to be consistent with the test above
  

  if ((select->em == select->sm) && (select->ec == select->sc)) return NULL;
  

  
  (Also, the extra parentheses in tests like ((a==b ) && (c==d)) are unnecessary, and IMHO distracting. Others disagree on the point however, so leave it if you really think it's clearer)
  

• I strongly suspect text_field_remove_selection() is wrong. Looking at the code it appears the first few messages are chopped off after character sc, and in the last message the bytes between sc and ec are thrown out. Apart from the fact that I think it won't work, it can also possibly cause a segfault, due to the
  

  	   msg->data[sc] = '\0';
  

  
  What I suspect is supposed to happen, is that in the first message, everything after sc is thrown out (unless it's also the last message, then bytes sc up to ec should be removed). Next, messages sm+1 up to and include ec-1 are deleted entirely, and finally in ec everything up to char ec is removed.
   
  The reason why you'd probably never notice this, is that the code is by accident correct when sc==ec, which is the case in every editable text field currently in the game. But if you write a general function, let's do it correctly.
  

• In text_field_keypress()
  

  +			   // what to do with cursor_line?
  

  
  Good question. I was hoping text_field_remove_selection() could deal with it. We probably have a problem with pasting text into a widget as well.
  * In update_delection()
  

  +	float displayed_font_y_size = DEFAULT_FONT_Y_LEN * w->size;
  

  
  I think this needs a floor()
  

  +	if (line > tf->nr_visible_lines) return;
  

  
  That should probably be >= .
  A more descriptive name of parameter flag would also be nice.
  

• In text_field_add_extended, this
  

  +	   for (i = 0; i < tf->nr_visible_lines; i++) if (tf->select.lines[i].msg $
  +	   {
  +		   tf->select.lines[i].msg = tf->select.lines[i - 1].msg;
  +		   tf->select.lines[i].chr = tf->buffer[tf->select.lines[i].msg].len;
  +	   }
  

  
  should really be realigned.
  

• Finally, a general comments: newlines are cheap. Inserting an empty line between blocks of related code can really improve readability.
  

[Alia]

Replies to comments (numbered in direct order).

+ means I'll do it (or at least try to do it

- means I'll ignore it.

 

[-] 1.

[+] 2.

[+] 3. It is not related to selection, I forgot to remove it, sorry

[+] 4.

[+] 5. It might be possible. skip_message must do the same thing as in draw_messages.

[+] 6. This is correct: if ((sm == em) && (sc > ec)). It was >= before, but I changed my mind.

However I haven't changed code in some other places, as I suspected

[+] 7. Oh, one line was deleted by accident: sc = 0 in the end of 'for' cycle (as in text_field_get_selected_text)

[+] 8. If it has to by rounded then floorf is better in this case.

Yes, 'if (line >= tf->nr_visible_lines) return;' is correct.

[-] 9.

[-] 10. Hmm, I'm not sure who are You to teach me how to write code :/

As I said in the beggining of this thread this is only a 'basic idea'.

By this I mean if someone will try to implement selection, he/she might look into this patch

if he/she have enough time to waste looking into such trash :/

Anyway, since now I'm going to ignore all irrelevant comments about text formatting.

[Grum]

[+] 8. If it has to by rounded then floorf is better in this case.

floorf was introduced in the C99 standard, so it's quite likely that using it won't work on certain compilers.

[-] 10. Hmm, I'm not sure who are You to teach me how to write code :/

A long time developer to this project. I'm not teaching you to write code, I was asking you to bring your patch in line with certain standards we use in the client code. I was under the impression that you might be willing to write a patch that could be applied to the client code without too much hassle, but

As I said in the beggining of this thread this is only a 'basic idea'.

By this I mean if someone will try to implement selection, he/she might look into this patch

it appears I was mistaken.

[Alia]

A long time developer to this project. I'm not teaching you to write code, I was asking you to bring your patch in line with certain standards we use in the client code.

 

Could you tell me where I can find those standards?

Otherwise, how am I supposed to guess what editor and preferences do you use, to make code look

good on your screen?

[Grum]

A good rule is to follow the conventions already present in the code.

 

As for the indentation: indenting wih tabs *only* has the advantage that it looks good in every editor, regardless of tab size[1]. Yes, there are places in the client code where this convention is broken, and if I come across such places while editing, I will fix them. Also note that I'm not calling you an idiot for not using tabs, I just pointed out that that's what's used in the client code, so if the patch were applied, it would have to be converted to using tabs. And of course I'd rather have you do that than doing it myself[2].

 

[1]: Yes indenting with spaces only looks good too. But you can't the width of spaces-indented code on most editors.

[ttlanhil]

floorf was introduced in the C99 standard, so it's quite likely that using it won't work on certain compilers.

It's #define'd in global.h for MSVC. If another compiler needs it, we can always #define again. Anything #define'd in global.h should theoretically be usable

[Alia]

Update: selection should work with scrolling now.

[Grum]

Great, thanks!

 

Is there more work you are planning on this patch? If so, let me know, otherwise I'll start merging it when I can find time at home.

[Alia]

Is there more work you are planning on this patch? If so, let me know, otherwise I'll start merging it when I can find time at home.

 

One thing has to be added: after block insertion and deletion message should be rewrapped.

But rewrapping 'eats' last characters from the message to insert '\r'. If we insert 1 character

it is not a problem, we can always keep 1 place for '\r'. But after insertion text with long lines

we lose part of the text. Therefor I'm planning to rewrite wrap_message in proper way.

[Grum]

Okay, sounds good. If it's just a matter of rewriting rewrap_message, could you do it in a separate patch?

[Alia]

1. At the moment there are no calls to rewrap_message after 'insert' and 'delete' at all.

I think they should be added in this patch.

 

2. I can make another patch for rewrap_message, but I'm going to use function append_char

from this one.

[Grum]

Okay, just build it on top of the current patch then. I'll wait until you're done.

[Alia]

Update: rewrapping message on insertion and deletion added.

Dynamic memory allocation for rewrap_message can be found here:

https://developer.berlios.de/patch/index.ph...p;group_id=1256

[Grum]

I've committed the first patch (not the realloc one), with some minor changes, mostly documentation related.(*) All in all it seems to work fine, except for the text input line in console. It looks like the highlighting position isn't reset there, or something, but when i type things, characters turn orange where they were selected on a previous message.

 

PS: nice job! I can finally paste text from EL to other windows!

Edited June 23, 2007 by Grum

[Alia]

There is odd situation with input_line in code design. For some reason we use different code for

editing input line when cursor is in it and when it is not. Further more, there is some common code

in functions 'text_input_handler' and 'root_key_to_input_field', which is called from the former one.

 

I'm quite sure it is possible to eliminate such duplications, that can cause more severe problems

than this with selection.

 

I think it might be done this way: first we check keys that should be treated differently and then pass

key to keypress_handler of input_line text_field.

 

P.S.

I hope client will support ctrl-ins / shift-ins as well as ctrl-c / ctrl-v