Hello all,
found some time for lyx today... and some bugs in the colors dialog. The
attached patch fixes
1. a crash when no color is selected and the "alter" button is pressed
2. when canceling the color selection dialog the color of the selected item is
changed to black
3. when canceling the preferences dialog after changing a color then the icon in the colors list is
not reverted. Moreover when pressing apply later then the color is really changed.
solutions:
1. disable the alter button if no item is selected and add an additional test
for a valid index.
2. add a test if the color returned from the color dialog is valid
3. move the color initialisation from the constructor to the update() method. I've read the mail
from richard (referenced in the source code) concerning this, but i don't understand why we should
not move parts of the initialization the constructor does into the update method. With this changes
the hack for fixing http://bugzilla.lyx.org/show_bug.cgi?id=3109 is not needed (even this is not
_the_ solution)
Comments are welcome, especially from Richard and Abdel concerning solution 3
If there are no objections i will commit on Wednesday.
Another issue on Windows:
At work i installed beta2 with interface language german and everything is german except for the
color selection dialog. At home when starting LyX from MSVC the interface language is english except
for the color selection dialog which is in german!
Bernhard
Index: src/frontends/qt4/QPrefs.cpp
===================================================================
--- src/frontends/qt4/QPrefs.cpp (revision 18229)
+++ src/frontends/qt4/QPrefs.cpp (working copy)
@@ -516,7 +516,8 @@
// FIXME: all of this initialization should be put into the controller.
// See
http://www.mail-archive.com/lyx-devel@lists.lyx.org/msg113301.html
- // for some discussion of why that is not trivial.
+ // for some discussion of why that is not trivia.l
+ QPixmap icon(32, 32);
for (int i = 0; i < Color::ignore; ++i) {
Color::color lc = static_cast<Color::color>(i);
if (lc == Color::none
@@ -532,19 +533,18 @@
|| lc == Color::ignore) continue;
lcolors_.push_back(lc);
- QColor color = QColor(guiApp->colorCache().get(lc));
- curcolors_.push_back(color.name());
- QPixmap coloritem(32, 32);
- coloritem.fill(color);
// This is not a memory leak:
- /*QListWidgetItem * newItem =*/ new
QListWidgetItem(QIcon(coloritem),
+ /*QListWidgetItem * newItem =*/ new QListWidgetItem(QIcon(icon),
toqstr(lcolor.getGUIName(lc)), lyxObjectsLW);
}
- newcolors_ = curcolors_;
+ curcolors_.resize(lcolors_.size());
+ newcolors_.resize(lcolors_.size());
// End initialization
connect(colorChangePB, SIGNAL(clicked()),
this, SLOT(change_color()));
+ connect(lyxObjectsLW, SIGNAL(itemSelectionChanged()),
+ this, SLOT(change_lyxobjects_selection()));
connect(lyxObjectsLW, SIGNAL(itemActivated(QListWidgetItem*)),
this, SLOT(change_color()));
}
@@ -553,32 +553,36 @@
void PrefColors::apply(LyXRC & /*rc*/) const
{
for (unsigned int i = 0; i < lcolors_.size(); ++i) {
- if (curcolors_[i]!=newcolors_[i])
+ if (curcolors_[i] != newcolors_[i]) {
form_->controller().setColor(lcolors_[i],
fromqstr(newcolors_[i]));
+ }
}
- // HACK The following line is needed because the values are not
- // re-initialized in ControlPrefs::initialiseParams but are only
- // initialized in the constructor. But the constructor is only called
- // once, when the dialog if first created, and is not called again when
- // Tools > Preferences is selected a second time: It's just called up
- // from memory. [See QDialogView::show(): if (!form) { build(); }.]
- curcolors_ = newcolors_;
}
-// FIXME The fact that this method is empty is also a symptom of the
-// problem here.
void PrefColors::update(LyXRC const & /*rc*/)
{
+ for (unsigned int i = 0; i < lcolors_.size(); ++i) {
+ QColor color = QColor(guiApp->colorCache().get(lcolors_[i]));
+ QPixmap coloritem(32, 32);
+ coloritem.fill(color);
+ lyxObjectsLW->item(i)->setIcon(QIcon(coloritem));
+ newcolors_[i] = curcolors_[i] = color.name();
+ }
+ change_lyxobjects_selection();
}
void PrefColors::change_color()
{
int const row = lyxObjectsLW->currentRow();
- QString color = newcolors_[row];
+
+ // just to be sure
+ if (row < 0) return;
+
+ QString const color = newcolors_[row];
QColor c(QColorDialog::getColor(QColor(color), qApp->focusWidget()));
- if (c.name() != color) {
+ if (c.isValid() && c.name() != color) {
newcolors_[row] = c.name();
QPixmap coloritem(32, 32);
coloritem.fill(c);
@@ -588,7 +592,12 @@
}
}
+void PrefColors::change_lyxobjects_selection()
+{
+ colorChangePB->setDisabled(lyxObjectsLW->currentRow() < 0);
+}
+
/////////////////////////////////////////////////////////////////////
//
// PrefCygwinPath
Index: src/frontends/qt4/QPrefs.h
===================================================================
--- src/frontends/qt4/QPrefs.h (revision 18229)
+++ src/frontends/qt4/QPrefs.h (working copy)
@@ -156,13 +156,14 @@
private Q_SLOTS:
void change_color();
+ void change_lyxobjects_selection();
private:
std::vector<Color_color> lcolors_;
// FIXME the use of mutable here is required due to the
// fact that initialization is not done in the controller
// but in the constructor.
- mutable std::vector<QString> curcolors_;
+ std::vector<QString> curcolors_;
std::vector<QString> newcolors_;
};
