On 04/20/2012 03:49 AM, Scott Kostyshak wrote:
go to document settings > local layout
enter a space
the "bug" is that you have to validate. This seems unnecessary.
Suppose someone accidentally entered some text (maybe they thought it
was the preamble) and then realized he was in the wrong spot and
erased everything. If an unobservant (doesn't see the text "Please
validate!") user doesn't realize that he has to validate an empty
entry, he might just click a different tab. If he does this, and makes
further changes, he will be unable to press the okPB and has to figure
out that he needs to go back to local layout and validate. Further,
even if the user validates an empty string, "Use Class Defaults" is
now the default button and the user might wonder if he just changed
something by accident.
This patch makes it so you do not have to validate an empty entry. It
does this by moving the check for an empty entry from validate() to
textChanged(). I keep a simple check of an empty string in validate()
because validate() is called when you open document settings. If this
check were removed, and if you were to enter a few empty lines and
click on OK and then go back to document settings you would see
"Layout is valid!". This is not that bad because it is true that it's
valid, but this text didn't show before so it would be inconsistent.
An alternative patch would force the user to validate before changing
the tab. A different alternative patch would not disable the okPB if
the text has not been validated but when the okPB is pressed it would
display a message along the lines of <<you need to go to the "local
layout" tab and press the "validate" button>>
Comments?
This seems sensible, overall. Minor comments below.
diff --git a/src/frontends/qt4/GuiDocument.cpp
b/src/frontends/qt4/GuiDocument.cpp
index 9f3f7ea..bd0dd10 100644
--- a/src/frontends/qt4/GuiDocument.cpp
+++ b/src/frontends/qt4/GuiDocument.cpp
@@ -606,12 +606,22 @@ void LocalLayout::apply(BufferParams& params)
void LocalLayout::textChanged()
{
- static const QString unknown = qt_("Press button to check validity...");
-
- is_valid_ = false;
- validLB->setText(unknown);
- validatePB->setEnabled(true);
- convertPB->setEnabled(false);
+ string const layout =
+ fromqstr(locallayoutTE->document()->toPlainText().trimmed());
+ if (layout.empty()) {
+ is_valid_ = true;
+ validatePB->setEnabled(false);
+ validLB->setText("");
+ convertPB->hide();
+ convertLB->hide();
+ } else {
It's a micro-optimization, but it might also make it more understandable
not to do this every time, but to check is_valid_ first: If that's
false, we shouldn't need to reset the text, etc., because it should
already be correct. So the idea is:
} else if (!is_valid) {
+ static const QString unknown = qt_("Press button to check
validity...");
+
+ is_valid_ = false;
+ validLB->setText(unknown);
+ validatePB->setEnabled(true);
+ convertPB->setEnabled(false);
+ }
changed();
}
@@ -649,13 +659,7 @@ void LocalLayout::validate() {
string const layout =
fromqstr(locallayoutTE->document()->toPlainText().trimmed());
- if (layout.empty()) {
- is_valid_ = true;
- validatePB->setEnabled(false);
- validLB->setText("");
- convertPB->hide();
- convertLB->hide();
- } else {
+ if (!layout.empty()) {
TextClass::ReturnValues const ret = TextClass::validate(layout);
is_valid_ = (ret == TextClass::OK) || (ret ==
TextClass::OK_OLDFORMAT);
validatePB->setEnabled(false);
