[haiku-commits] Re: haiku: hrev47318 - src/apps/terminal
- From: John Scipione <jscipione@xxxxxxxxx>
- To: "haiku-commits@xxxxxxxxxxxxx" <haiku-commits@xxxxxxxxxxxxx>
- Date: Thu, 5 Jun 2014 18:05:46 -0400
On Thu, Jun 5, 2014 at 5:55 PM, Ingo Weinhold <ingo_weinhold@xxxxxx> wrote:
> On 05.06.2014 22:22, jscipione@xxxxxxxxx wrote:
>> ############################################################################
>>
>> Commit: b84955c73892d942d2b53e7a992b63e774e52789
>> URL: http://cgit.haiku-os.org/haiku/commit/?id=b84955c
>> Author: John Scipione <jscipione@xxxxxxxxx>
>> Date: Thu Jun 5 20:09:52 2014 UTC
>>
>> TermViewStates: Check if fView is NULL before using it.
>
>
> Could you please try to at least somewhat understand code before making
> changes? fView is set in the base class constructor and never changed.
> Please revert.
I don't think using a pointer without checking if it is NULL is a very
good idea even if you think it can't be NULL, even so, I reverted the
change.
>> ############################################################################
>>
>> Revision: hrev47318
>> Commit: 4c8d2387169eb503e9a4a347645dd37c238d1c8a
>> URL: http://cgit.haiku-os.org/haiku/commit/?id=4c8d238
>> Author: John Scipione <jscipione@xxxxxxxxx>
>> Date: Thu Jun 5 20:18:49 2014 UTC
>>
>> Ticket: https://dev.haiku-os.org/ticket/10902
>>
>> Fix #10902 Terminal crashes setting window size.
>>
>> Terminal crashes because fView is not connected to App Server when
>> this is called so calling fView->GetMouse() is not allowed.
>
> The interesting questions therefore are:
>
> 1) Why is the view not attached?
> 2) Why is it in the hyper link state?
>
> 1) is because the BTabView removes the non-selected tabs instead of just
> hiding them.
>
> The reason for 2) is that the tab was opened with Cmd+T (the bug is not
> reproducible when the tab is opened via menu item). Pressing Cmd causes
> hyper link state to be entered and switching to the new tab will leave the
> view in that state due to 1).
>
> Possible solutions:
>
> * TermView::_VisibleTextBufferChanged(): Call state hook only when attached
> to window. All other occurrences are safe as they are in BView hooks.
>
> * Leave the hyper link state when the view is detached from the window. A
> new dummy state could be active as long as the view is not attached, though
> using DefaultState would be harmless as well.
>
>> We could
>> perhaps check to make sure that fView is connected to App Server by
>> checking that fView->Window() != NULL, but, a better way to handle this
>> is to grab the already set fLastClickPoint variable from fView
>> which eliminates the need to grab the current mouse position again.
>
> fLastClickPoint is, as the name suggests, the point where the user last
> clicked (actually the point of the last MouseDown()), not the current mouse
> position.
>
>> Worst case scenario is that fLastClickPoint hasn't been set in which
>> case Terminal won't find anything to highlight or unhighlight.
>
> No, the worst case scenario is that the hyper link feature is now partially
> broken. E.g. pressing Cmd while the mouse is over a path will no longer
> highlight the path.
>
>> Also check to make sure that fView != NULL here for good measure.
I reverted this too. Feel free to fix the bug more properly.
Other related posts:
