Georg Baum wrote:
Abdelrazak Younes wrote:
Georg Baum wrote:
Am Samstag, 16. September 2006 18:47 schrieb Abdelrazak Younes:
I don't need to know. I am just moving code around and this should just
work. I am quite confident actually that it _will_ just work.
OK, suppose it works. Then there is still the problem that this is
supposed to be a cleanup, but obfuscates the code for somebody who knows
X selection better.
Such as you?
Partly. I do know a bit about it, but not enough to completely understand
the current selection code, in particular not why there is this static
xsel_cache_ variable. This is of course not your doing, but a real cleanup
would only be possible with understanding the existence of this static
variable.
It is not a static variable but a class member:
/// this is used to handle XSelection events in the right manner
struct {
CursorSlice cursor;
CursorSlice anchor;
bool set;
} xsel_cache_;
It is standard X terminology to request or clear a
selection, so a sensible cleanup IMO would be to rename
selectionRequested to requestSelection and selectionLost to
clearSelection.
Well, this new method touch _nothing_ that is related with X11
(following my cleanup). It just returns the current selection.
That is not true (xsel_cache_).
This structure has really nothing directly related to X except for the
first letter 'x'. Being used by the X selection framework does not mean
that it should be it's only purpose.
This
method should work for any platform, not only X11.
I really don't agree with that. I would like to mimics the X selection
feature in windows and Mac also.
Only X11 has a selection (at least in the sense of selectionRequested). For
internal purposes we have e.g. LCursor::selectionAsString.
See above.
No need for X knowledge, just need someone who test if the selection
works or not, that' all.
IMO you need X knowledge if it should be really a cleanup and not
arbitrary code shuffling around.
I think that's an unfair comment Georg.
I don't think it is unfair. You are changing code related to something you
don't use and don't know very well.
Perhaps you are under estimating me a bit? I've had a close look at the
code and at the xselection machinery in LyX.
IMO a real cleanup is only possible
with knowledge about the context. In your new name getStringSelection I
already see a misunderstanding (I know that you committed the other
variant, but I'll show you nevertheless): selectionRequested was only used
from the frontend. Your explanation of getStringSelection implies that you
think it could also be used for other purposes. That should not be done IMO
because of the internal cache.
Now you can argue that the current code is not clear either. I completely
agree with that, but that does not mean that everything else is better.
So you'd argue that a small cleanup (such as my patch) is not useful?
Perhaps you didn't see that the real objective of this cleanup was to
fix the calling of a frontend feature inside the kernel.
If it is just a question of
naming I am all ears. I've named that getStringSelection() because
that's exactly what it does: return the current selection as a string.
Nobody says that X terminology is the only useful one, yet it is still known
for years. Apart from that the method does not only return a string, but
also sets some internal static cache.
Man, I have acknowledged this and changed the naming. So you were right
on this. But I would have preferred that you state it directly instead
of dismissing my work entirely because I don't know X enough.
Abdel.
Georg
