Re: Spellchecker bugs (angus !)

[Angus Leeming]

John Levon wrote:

 
 OK, so we're not calling speller_-close() when the window is closed or
 Close is  pressed - bad, we didn't write out the accepted word list.
 
 dialogs call controller().stop() but this doesn't actually do anything
 interesting. Angus, how can we make it so that it will call -close()
 but nothing else will break ? -close() isn't idempotent for ispell,
 and  it doesn't look like we could safely destruct the speller_
 instance.
 
 I find this code a little confusing ...  I'm considering a rewrite.

So am I. In fact...

I'd suggest doing nothing to this code for a while. Attached is a NEW Dialog 
class that should make life a LOT easier. I'm pretty sure that this 
abstraction is flexible enough to be used by ALL dialogs and I find it 
coherent. Does it make sense to you too?

We can use this because the Dialogs class (that's frontends/Dialogs not 
frontends/controllers/Dialog) now stores a mapname, dialog and so can 
access each dialog easily. Moreover, this means that the rather obtuse code 
using signals to hide an inset dialog or update the dialogs on a buffer 
change can now go because we can simply access an individual dialog or loop 
over the container of all dialogs.

Still a work in progress, but it seems to hang together...

Note also that I'm going to move build() back into the xforms/qt/gnome code.

-- 
Angus
// -*- C++ -*-
/**
 * \file Dialog.C
 * This file is part of LyX, the document processor.
 * Licence details can be found in the file COPYING.
 *
 * \author Angus Leeming 
 *
 * Full author contact details are available in file CREDITS
 */

#include Dialog.h

#include ButtonControllerBase.h
#include ViewBase.h



Dialog::Dialog(LyXView  lv)
	: kernel_(lv)
{}


void Dialog::ApplyButton()
{
	apply();
	bc().apply();
}


void Dialog::OKButton()
{
	is_closing_ = true;
	apply();
	is_closing_ = false;
	hide();
	bc().ok();
}


void Dialog::CancelButton()
{
	hide();
	bc().cancel();
}


void Dialog::RestoreButton()
{
	kernel().updateDialog(name_);
	bc().restore();
}


void Dialog::show(string const  data)
{
	if (impl().isBufferDependent()  !kernel().isBufferAvailable())
		return;

	impl().initialiseParams(data);

	bc().readOnly(kernel().isBufferReadonly());
	view().show();

	// The widgets may not be valid, so refresh the button controller
	bc().refresh();
}


void Dialog::update(string const  data)
{
	if (impl().isBufferDependent()  !kernel().isBufferAvailable())
		return;

	impl().initialiseParams(data);

	bc().readOnly(kernel().isBufferReadonly());
	view().update();

	// The widgets may not be valid, so refresh the button controller
	bc().refresh();
}


void Dialog::hide()
{
	impl().clearParams();
	view().hide();
}


void Dialog::apply()
{
	if (kernel().isBufferReadonly())
		return;

	view().apply();
	impl().dispatchParams();

	if (impl().disconnectOnApply()  !is_closing_) {
 		impl().initialiseParams(string());
		view().update();
	}
}


bool Dialog::isVisible() const
{
	return view().isVisible();
}


void Dialog::redraw()
{
	view().redraw();
}


ButtonControllerBase  Dialog::bc() const
{
	lyx::Assert(bc_ptr_.get());
	return *bc_ptr_.get();
}


ViewBase  Dialog::view() const
{
	lyx::Assert(view_ptr_.get());
	return *view_ptr_.get();
}


void Dialog::setView(ViewBase * v)
{
	lyx::Assert(v  !view_ptr_.get());
	view_ptr_.reset(v);
}


void Dialog::setImpl(Impl * i)
{
	lyx::Assert(i  !impl_ptr_.get());
	impl_ptr_.reset(i);
}


void Dialog::setButtonController(ButtonControllerBase * bc)
{
	lyx::Assert(bc  !bc_ptr_.get());
	bc_ptr_.reset(bc);
}


Dialog::Impl::Impl(Dialog  parent)
	: parent_(parent)
{}

// -*- C++ -*-
/**
 * \file Dialog.h
 * This file is part of LyX, the document processor.
 * Licence details can be found in the file COPYING.
 *
 * \author Angus Leeming
 *
 * Full author contact details are available in file CREDITS
 */

#ifndef DIALOG_H
#define DIALOG_H


#include Kernel.h
#include LString.h
#include boost/utility.hpp
#include boost/scoped_ptr.hpp


class ButtonControllerBase;
class LyXView;
class ViewBase;


class Dialog : boost::noncopyable {
public:
	///
	Dialog(LyXView );

	/** These methods are publicly accessible because they are invoked
	by the View.
	*/
	///
	void ApplyButton();
	///
	void OKButton();
	///
	void CancelButton();
	///
	void RestoreButton();
	
	/** These methods are publicly accessible because they are invoked
	 *  by the Dialogs class.
	 */
	/** Some dialogs, eg the Tabular or Preferences dialog, can extract
	the information they require from the kernel. These dialogs will
	probably be passed an empty string by the calling Dailogs class.
	The inset dialogs, however, require information specific to
	an individual inset. This information will be encoded in the
	string and must be translated into a set of parameters that
	can be updated from the dialog.
	 */
	void show(string const  data = string());
	///
	void update(string const  data = string());
	///
	void hide();
	///
	bool isVisible() const;
	/// (Eg, the 

Re: Spellchecker bugs (angus !)

[John Levon]

On Sun, Feb 16, 2003 at 05:40:49PM +, Angus Leeming wrote:

 I'd suggest doing nothing to this code for a while.

Too late ...

 Attached is a NEW Dialog class that should make life a LOT easier.

Actually most of my problems were unrelated  to the framework ...
can we leave this for a short while ? The reason I ask is because I've
nearly finished with the reworking and fixed a ton of bugs (at least 10
real and theoretical bugs/bad UI) in the spell code, and I want the same
patch to go in to 1.3.1.

I've come up with a hack for the problem I mentioned so once cvs is back
I can post my final version for testing. After that I'll have a look at
your more radical cleanups ... Any simplification so my poor brain can
follow the framework code gets pre-approved from me ;)

regards
john

Re: Spellchecker bugs (angus !)

[Angus Leeming]

John Levon wrote:

> 
> OK, so we're not calling speller_->close() when the window is closed or
> Close is  pressed - bad, we didn't write out the accepted word list.
> 
> dialogs call controller().stop() but this doesn't actually do anything
> interesting. Angus, how can we make it so that it will call ->close()
> but nothing else will break ? ->close() isn't idempotent for ispell,
> and  it doesn't look like we could safely destruct the speller_
> instance.
> 
> I find this code a little confusing ...  I'm considering a rewrite.

So am I. In fact...

I'd suggest doing nothing to this code for a while. Attached is a NEW Dialog 
class that should make life a LOT easier. I'm pretty sure that this 
abstraction is flexible enough to be used by ALL dialogs and I find it 
coherent. Does it make sense to you too?

We can use this because the Dialogs class (that's frontends/Dialogs not 
frontends/controllers/Dialog) now stores a map and so can 
access each dialog easily. Moreover, this means that the rather obtuse code 
using signals to hide an inset dialog or update the dialogs on a buffer 
change can now go because we can simply access an individual dialog or loop 
over the container of all dialogs.

Still a work in progress, but it seems to hang together...

Note also that I'm going to move build() back into the xforms/qt/gnome code.

-- 
Angus
// -*- C++ -*-
/**
 * \file Dialog.C
 * This file is part of LyX, the document processor.
 * Licence details can be found in the file COPYING.
 *
 * \author Angus Leeming 
 *
 * Full author contact details are available in file CREDITS
 */

#include "Dialog.h"

#include "ButtonControllerBase.h"
#include "ViewBase.h"



Dialog::Dialog(LyXView & lv)
	: kernel_(lv)
{}


void Dialog::ApplyButton()
{
	apply();
	bc().apply();
}


void Dialog::OKButton()
{
	is_closing_ = true;
	apply();
	is_closing_ = false;
	hide();
	bc().ok();
}


void Dialog::CancelButton()
{
	hide();
	bc().cancel();
}


void Dialog::RestoreButton()
{
	kernel().updateDialog(name_);
	bc().restore();
}


void Dialog::show(string const & data)
{
	if (impl().isBufferDependent() && !kernel().isBufferAvailable())
		return;

	impl().initialiseParams(data);

	bc().readOnly(kernel().isBufferReadonly());
	view().show();

	// The widgets may not be valid, so refresh the button controller
	bc().refresh();
}


void Dialog::update(string const & data)
{
	if (impl().isBufferDependent() && !kernel().isBufferAvailable())
		return;

	impl().initialiseParams(data);

	bc().readOnly(kernel().isBufferReadonly());
	view().update();

	// The widgets may not be valid, so refresh the button controller
	bc().refresh();
}


void Dialog::hide()
{
	impl().clearParams();
	view().hide();
}


void Dialog::apply()
{
	if (kernel().isBufferReadonly())
		return;

	view().apply();
	impl().dispatchParams();

	if (impl().disconnectOnApply() && !is_closing_) {
 		impl().initialiseParams(string());
		view().update();
	}
}


bool Dialog::isVisible() const
{
	return view().isVisible();
}


void Dialog::redraw()
{
	view().redraw();
}


ButtonControllerBase & Dialog::bc() const
{
	lyx::Assert(bc_ptr_.get());
	return *bc_ptr_.get();
}


ViewBase & Dialog::view() const
{
	lyx::Assert(view_ptr_.get());
	return *view_ptr_.get();
}


void Dialog::setView(ViewBase * v)
{
	lyx::Assert(v && !view_ptr_.get());
	view_ptr_.reset(v);
}


void Dialog::setImpl(Impl * i)
{
	lyx::Assert(i && !impl_ptr_.get());
	impl_ptr_.reset(i);
}


void Dialog::setButtonController(ButtonControllerBase * bc)
{
	lyx::Assert(bc && !bc_ptr_.get());
	bc_ptr_.reset(bc);
}


Dialog::Impl::Impl(Dialog & parent)
	: parent_(parent)
{}

// -*- C++ -*-
/**
 * \file Dialog.h
 * This file is part of LyX, the document processor.
 * Licence details can be found in the file COPYING.
 *
 * \author Angus Leeming
 *
 * Full author contact details are available in file CREDITS
 */

#ifndef DIALOG_H
#define DIALOG_H


#include "Kernel.h"
#include "LString.h"
#include 
#include 


class ButtonControllerBase;
class LyXView;
class ViewBase;


class Dialog : boost::noncopyable {
public:
	///
	Dialog(LyXView &);

	/** These methods are publicly accessible because they are invoked
	by the View.
	*/
	///
	void ApplyButton();
	///
	void OKButton();
	///
	void CancelButton();
	///
	void RestoreButton();
	
	/** These methods are publicly accessible because they are invoked
	 *  by the Dialogs class.
	 */
	/** Some dialogs, eg the Tabular or Preferences dialog, can extract
	the information they require from the kernel. These dialogs will
	probably be passed an empty string by the calling Dailogs class.
	The inset dialogs, however, require information specific to
	an individual inset. This information will be encoded in the
	string and must be translated into a set of parameters that
	can be updated from the dialog.
	 */
	void show(string const & data = string());
	///
	void update(string const & data = string());
	///
	void hide();
	///
	bool isVisible() const;
	/// 

Re: Spellchecker bugs (angus !)

[John Levon]

On Sun, Feb 16, 2003 at 05:40:49PM +, Angus Leeming wrote:

> I'd suggest doing nothing to this code for a while.

Too late ...

> Attached is a NEW Dialog class that should make life a LOT easier.

Actually most of my problems were unrelated  to the framework ...
can we leave this for a short while ? The reason I ask is because I've
nearly finished with the reworking and fixed a ton of bugs (at least 10
real and theoretical bugs/bad UI) in the spell code, and I want the same
patch to go in to 1.3.1.

I've come up with a hack for the problem I mentioned so once cvs is back
I can post my final version for testing. After that I'll have a look at
your more radical cleanups ... Any simplification so my poor brain can
follow the framework code gets pre-approved from me ;)

regards
john

Spellchecker bugs (angus !)

[John Levon]

OK, so we're not calling speller_-close() when the window is closed or
Close is  pressed - bad, we didn't write out the accepted word list.

dialogs call controller().stop() but this doesn't actually do anything
interesting. Angus, how can we make it so that it will call -close()
but nothing else will break ? -close() isn't idempotent for ispell,
and  it doesn't look like we could safely destruct the speller_
instance.

I find this code a little confusing ...  I'm considering a rewrite.

john

Spellchecker bugs (angus !)

[John Levon]

OK, so we're not calling speller_->close() when the window is closed or
Close is  pressed - bad, we didn't write out the accepted word list.

dialogs call controller().stop() but this doesn't actually do anything
interesting. Angus, how can we make it so that it will call ->close()
but nothing else will break ? ->close() isn't idempotent for ispell,
and  it doesn't look like we could safely destruct the speller_
instance.

I find this code a little confusing ...  I'm considering a rewrite.

john