Commit 9d8e4729 authored by Tomaz  Canabrava's avatar Tomaz Canabrava Committed by Kurt Hindenburg

Fix Profile "Set default" not working

This was harder than I tought. Because of the static initialization
static linkage order, we ended up having two different values of
a static variable on the code, that where supposedly only one.

https://stackoverflow.com/questions/26547454/static-variable-is-initialized-twice

The choosen change: Use OBJECT instead of STATIC for linking also
made me fix a few other profile headers to use the correct folder.
parent 3d66c5f5
......@@ -225,8 +225,8 @@ target_link_libraries(konsoleprivate
konsolecolorscheme
keyboardtranslator
konsolehelpers
konsoleprofile
konsolesession
konsoleprofile
${konsole_LIBS}
)
......@@ -256,7 +256,12 @@ set(konsole_KDEINIT_SRCS
ecm_add_app_icon(kdeinit_konsole ICONS ${ICONS_SRCS})
kf5_add_kdeinit_executable(konsole ${konsole_KDEINIT_SRCS})
target_link_libraries(kdeinit_konsole konsoleprivate KF5::XmlGui KF5::WindowSystem KF5::Bookmarks
target_link_libraries(kdeinit_konsole
konsoleprofile
konsoleprivate
KF5::XmlGui
KF5::WindowSystem
KF5::Bookmarks
KF5::I18n
KF5::KIOWidgets
KF5::NotifyConfig
......@@ -290,6 +295,11 @@ add_library(konsolepart MODULE ${konsolepart_PART_SRCS})
generate_export_header(konsolepart BASE_NAME konsole)
kcoreaddons_desktop_to_json(konsolepart ../desktop/konsolepart.desktop)
set_target_properties(konsolepart PROPERTIES DEFINE_SYMBOL KONSOLE_PART)
target_link_libraries(konsolepart KF5::Parts KF5::XmlGui konsoleprivate)
target_link_libraries(konsolepart
KF5::Parts
KF5::XmlGui
konsoleprofile
konsoleprivate
)
install(TARGETS konsolepart DESTINATION ${KDE_INSTALL_PLUGINDIR})
......@@ -22,7 +22,7 @@
#ifndef SHOULDAPPLYPROPERTY_H
#define SHOULDAPPLYPROPERTY_H
#include "Profile.h"
#include "profile/Profile.h"
namespace Konsole
{
......
add_library(konsoleprofile
STATIC
OBJECT
  • This breaks the build with older CMake versions.

    Rule of thumb: NEVER use target_link_libraries when using 'OBJECT'

    [   44s] CMake Error at src/profile/CMakeLists.txt:17 (target_link_libraries):
    [   44s]   Object library target "konsoleprofile" may not link to anything.
    Edited by Christophe Giboudeaux
  • This is in 20.12 as well which will be released in 2 weeks. What cmake versions are failing?

    @tcanabrava

  • it will fail with any CMake version <= 3.12

  • I have time to work on this this week

  • cmake version 3.12.3 was released Nov 21, 2018 - every system I have has a later version. We could always set a cmake min version if needed.

  • Should the cmake min version be update to 3.13?

  • if we can, I'd go for it. I'v had a few bugs while splitting hte libraries on konsole because of static initialization fiasco, I need more time to look at that and do a proper separation.

Please register or sign in to reply
Profile.cpp
ProfileCommandParser.cpp
ProfileGroup.cpp
......
......@@ -42,6 +42,7 @@
#include "ProfileReader.h"
#include "ProfileWriter.h"
#include "ProfileGroup.h"
#include "ProfileModel.h"
using namespace Konsole;
......@@ -491,8 +492,8 @@ bool ProfileManager::deleteProfile(Profile::Ptr profile)
void ProfileManager::setDefaultProfile(const Profile::Ptr &profile)
{
Q_ASSERT(_profiles.contains(profile));
_defaultProfile = profile;
ProfileModel::instance()->setDefault(profile);
}
void ProfileManager::saveDefaultProfile()
......
......@@ -5,6 +5,7 @@
#include <KLocalizedString>
#include <QIcon>
#include <QDebug>
using namespace Konsole;
......@@ -27,6 +28,8 @@ ProfileModel::ProfileModel()
int ProfileModel::rowCount(const QModelIndex &unused) const
{
Q_UNUSED(unused);
// All profiles plus the default profile that's always at index 0 on
return m_profiles.count();
}
......@@ -58,12 +61,17 @@ QVariant ProfileModel::data(const QModelIndex& idx, int role) const
return {};
}
QExplicitlySharedDataPointer<Profile> profile = m_profiles.at(idx.row());
if (idx.row() > m_profiles.count()) {
return {};
}
auto profile = m_profiles.at(idx.row());
switch (idx.column()) {
case NAME: {
switch (role) {
case Qt::DisplayRole: return QStringLiteral("%1%2").arg(profile->name(), (idx.row() == 0 ? i18n("(Default)") : QString()));
case Qt::DisplayRole: {
return QStringLiteral("%1%2").arg(profile->name(), (idx.row() == 0 ? i18n("(Default)") : QString()));
}
case Qt::DecorationRole: return QIcon::fromTheme(profile->icon());
case Qt::FontRole: {
// Default Profile
......@@ -142,21 +150,30 @@ void ProfileModel::populate()
endResetModel();
}
void ProfileModel::add(QExplicitlySharedDataPointer<Konsole::Profile> profile)
void ProfileModel::add(QExplicitlySharedDataPointer<Profile> profile)
{
// The model is too small for this to matter.
Q_UNUSED(profile);
populate();
}
void ProfileModel::remove(QExplicitlySharedDataPointer<Konsole::Profile> profile)
void ProfileModel::remove(QExplicitlySharedDataPointer<Profile> profile)
{
// The model is too small for this to matter.
Q_UNUSED(profile);
populate();
}
void ProfileModel::update(QExplicitlySharedDataPointer<Konsole::Profile> profile)
void ProfileModel::setDefault(QExplicitlySharedDataPointer<Profile> profile)
{
if (m_profiles.count()) {
m_profiles.removeFirst();
m_profiles.prepend(profile);
}
emit dataChanged(index(0, 0), index(0, COLUMNS-1), {Qt::DisplayRole});
}
void ProfileModel::update(QExplicitlySharedDataPointer<Profile> profile)
{
int row = m_profiles.indexOf(profile);
dataChanged(index(row, 0), index(row, COLUMNS-1));
......
......@@ -19,6 +19,7 @@ public:
void add(QExplicitlySharedDataPointer<Profile> profile);
void remove(QExplicitlySharedDataPointer<Profile> profile);
void update(QExplicitlySharedDataPointer<Profile> profile);
void setDefault(QExplicitlySharedDataPointer<Profile> profile);
int rowCount(const QModelIndex& parent) const override;
int columnCount(const QModelIndex& parent) const override;
......@@ -29,7 +30,6 @@ public:
private:
QList<QExplicitlySharedDataPointer<Profile>> m_profiles;
ProfileModel();
};
}
......
......@@ -43,7 +43,7 @@
#include "history/HistoryTypeNone.h"
#include "profile/ProfileManager.h"
#include "ProfileCommandParser.h"
#include "profile/ProfileCommandParser.h"
#include "session/Session.h"
#include "session/SessionController.h"
......
......@@ -40,11 +40,10 @@ using namespace Konsole;
ProfileSettings::ProfileSettings(QWidget* parent)
: QWidget(parent)
, m_profileModel(ProfileModel::instance())
{
setupUi(this);
profilesList->setModel(m_profileModel);
profilesList->setModel(ProfileModel::instance());
profilesList->setItemDelegateForColumn(ProfileModel::SHORTCUT, new ShortcutItemDelegate(this));
// double clicking the profile name opens the profile edit dialog
......
......@@ -38,7 +38,6 @@ class QStandardItemModel;
namespace Konsole {
class Profile;
class ProfileModel;
/**
* A dialog which lists the available types of profiles and allows
......@@ -91,8 +90,6 @@ private:
// updates the profile table to be in sync with the
// session manager
void populateTable();
ProfileModel *m_profileModel;
};
}
......
......@@ -59,7 +59,7 @@
#include "keyboardtranslator/KeyboardTranslatorManager.h"
#include "KeyBindingEditor.h"
#include "ProfileManager.h"
#include "profile/ProfileManager.h"
#include "ShellCommand.h"
#include "WindowSystemInfo.h"
#include "FontDialog.h"
......
......@@ -30,8 +30,9 @@
#include "colorscheme/ColorSchemeEditor.h"
#include "colorscheme/ColorSchemeViewDelegate.h"
#include "Profile.h"
#include "ProfileGroup.h"
#include "profile/Profile.h"
#include "profile/ProfileGroup.h"
#include "Enumeration.h"
#include "keyboardtranslator/KeyboardTranslatorManager.h"
#include "FontDialog.h"
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment