[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 21:32:16 -0400
On Thursday, June 5, 2014, John Scipione <jscipione@xxxxxxxxx> wrote:
> On Thu, Jun 5, 2014 at 5:55 PM, Ingo Weinhold <ingo_weinhold@xxxxxx
> <javascript:;>> wrote:
> > On 05.06.2014 22:22, jscipione@xxxxxxxxx <javascript:;> wrote:
> >>
> ############################################################################
> >>
> >> Commit: b84955c73892d942d2b53e7a992b63e774e52789
> >> URL: http://cgit.haiku-os.org/haiku/commit/?id=b84955c
> >> Author: John Scipione <jscipione@xxxxxxxxx <javascript:;>>
> >> 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 <javascript:;>>
> >> 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.
>
Sorry that came off a little flippant. I guess my solution was a bit
premature. Thank you for you help, let me sleep on it and take another
crack at it in the morning.
Other related posts:
