Abdelrazak Younes wrote:
> Georg Baum wrote:
>> 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_;
Sorry, I mixed it up with the static sel variable that is now gone, but in
this context the difference is not important: xsel_cache_ stores some
internal state.
>>>> 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.
Agreed.
>>> This
>>> method should work for any platform, not only X11.
>
> I really don't agree with that.
I guess you answered to my quoted text below?
> I would like to mimics the X selection
> feature in windows and Mac also.
That was not clear to me. I thought you meant requestSelection to be a
general purpose method for getting the selection as string.
>> 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?
That is of course possible.
> I've had a close look at the
> code and at the xselection machinery in LyX.
Fine.
>> 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?
No. My argument is that you can't judge if it is a cleanup or not if you
don't know the context. If you say that you know enough about X selection
to do this judgement then I believe you, but to me it did not look like
that, since you stated twice that you don't need X knowledge. I admit that
this double statement probably provoked me to formulate my concerns more
drastically than originally intended.
> Perhaps you didn't see that the real objective of this cleanup was to
> fix the calling of a frontend feature inside the kernel.
I did see, and I agree with that goal.
>>> 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.
I did not dismiss your work entirely, please reread my older messages. I
only pointed out that it might not be the cleanup you think it is, and I
said more than once that I don't know the full picture either. Otherwise I
would have complained about a specific problem. Please don't think that
everybody here is engaged in fighting you.
Georg
