Add GUI config tool support for fcitx-rime. #40

xuzhao9 · 2018-05-20T22:46:55Z

If you only want to test the GUI tool, please check out the following repo:
https://github.com/xuzhao9/fcitx-rime-config

wengxt · 2018-05-21T16:27:24Z

CMakeLists.txt

@@ -29,6 +29,11 @@ configure_file(
 add_custom_target(uninstall
    COMMAND ${CMAKE_COMMAND} -P ${CMAKE_CURRENT_BINARY_DIR}/cmake_uninstall.cmake)

+option(QT5GUI "Build the Rime Config GUI Tool" ON)


call it ENABLE_XXXX

wengxt · 2018-05-21T16:28:09Z

gui/CMakeLists.txt

@@ -0,0 +1,8 @@
+set(CMAKE_CXX_STANDARD 14)


use set_target_properties for this

wengxt · 2018-05-21T16:28:50Z

gui/CMakeLists.txt

@@ -0,0 +1,8 @@
+set(CMAKE_CXX_STANDARD 14)
+find_package(FcitxQt5WidgetsAddons 1.0.0 REQUIRED)
+find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED Core Widgets Concurrent)


REQUIRED_QT_VERSION is not defined. Also do you need Concurrent?

wengxt · 2018-05-21T16:29:24Z

gui/src/CMakeLists.txt

+  ${FRCU_HDRS}
+  ${FRCU_UIS})
+
+include_directories(


not needed. As long as you use new style target_link_libraries

wengxt · 2018-05-21T16:29:53Z

gui/src/CMakeLists.txt

+  ${RIME_INCLUDE_DIRS}
+  ${CMAKE_CURRENT_BINARY_DIR})
+
+link_directories(


This is not needed, too.

wengxt · 2018-05-21T16:34:38Z

gui/src/ConfigMain.h

@@ -0,0 +1,54 @@
+#pragma once


I prefer ifndef

wengxt · 2018-05-21T16:35:57Z

gui/src/Model.cpp

+  }
+
+  void FcitxRimeConfigDataModel::sortKeys() {
+    sortSingleKeySet(this->toggle_keys);


why you have this-> here?

wengxt · 2018-05-21T16:37:09Z

gui/src/Model.cpp

+
+
+  // convert keyseq to state and sym
+  FcitxKeySeq::FcitxKeySeq(const char* keyseq) {


Why dup the code? Is there any api not available from libfcitx?

wengxt · 2018-05-21T16:39:07Z

gui/src/fcitx-utils/cutf8.cpp

@@ -0,0 +1,399 @@
+/*


this is availabe in fcitx4.

wengxt · 2018-05-21T16:42:12Z

gui/src/fcitx-utils/flags.h

@@ -0,0 +1,91 @@
+/*


since you're using qt, consider use QFlags instead. so you don't need cxx14.

wengxt · 2018-05-24T05:05:41Z

gui/src/Common.h

@@ -0,0 +1,20 @@
+#ifndef FCITX_RIME_CONFIG_COMMON_H


Please add copyright header

wengxt · 2018-05-24T05:06:18Z

gui/src/ConfigMain.h

+    Q_OBJECT
+  public:
+    explicit ConfigMain(QWidget* parent = 0);
+    QString title();


Add override after virtual functions.

wengxt · 2018-05-24T05:06:46Z

gui/src/ConfigMain.h

+    void availIMSelectionChanged();
+    void activeIMSelectionChanged();
+  private:
+    Ui::MainUI* m_ui;


Personally, I prefer subclass over member for UI class.

wengxt · 2018-05-24T05:11:14Z

gui/src/Main.h

+public:
+    Q_PLUGIN_METADATA(IID "FcitxQtConfigUIFactoryInterface_iid" FILE "fcitx-rime-config.json")
+    explicit FcitxRimeConfigTool(QObject* parent = 0);
+    virtual QString name();


use override

wengxt · 2018-05-24T05:11:36Z

gui/src/Model.h

+#include <qkeysequence.h>
+#include <QFlags>
+
+#define MAX_SHORTCUTS 3


maybe constexpr.

wengxt · 2018-05-24T05:13:12Z

gui/src/rime-utils/FcitxRimeConfig.h

+#define FCITX_RIME_CONFIG_SUFFIX ".yaml"
+#define FCITX_RIME_SCHEMA_SUFFIX ".schema.yaml"
+
+#ifdef __cplusplus


is there any point to check cxx here?

wengxt · 2018-05-24T05:13:49Z

gui/src/rime-utils/FcitxRimeConfig.c

+#include "fcitx-utils/utils.h"
+#include <stdio.h>
+
+#ifdef __cplusplus


you can use c++ in this file, I guess?

Can I? The rime api is C API, so I have to call using C interface.

wengxt · 2018-05-24T05:15:00Z

gui/src/Model.h

+
+#include <rime_api.h>
+
+#include "fcitx-utils/keysym.h"


I prefer you don't use "fcitx-utils/" sub directory here.

wengxt · 2018-05-24T05:15:12Z

gui/src/Model.h

+
+#include "fcitx-utils/keysym.h"
+#include "fcitx-utils/keysymgen.h"
+#include <qkeysequence.h>


QKeySequence

wengxt · 2018-05-25T05:14:18Z

gui/src/ConfigMain.cpp

@@ -16,39 +32,38 @@

 namespace fcitx_rime {
  ConfigMain::ConfigMain(QWidget* parent) :
-    FcitxQtConfigUIWidget(parent), m_ui(new Ui::MainUI),
+    FcitxQtConfigUIWidget(parent), Ui::MainUI(),


Ui::MainUI(), is not necessary

wengxt · 2018-05-25T05:15:48Z

gui/src/FcitxRimeConfig.cpp

+// key: value of the act_type, Page_Up, Page_Down, etc
+// value: shortcut
+void FcitxRimeConfigSetKeyBindingSet(RimeConfig* config, int type, const char* key, const char** shortcuts, size_t shortcut_size) {
+  RimeConfigIterator* iter = (RimeConfigIterator*) fcitx_utils_malloc0(sizeof(RimeConfigIterator));


do you need this on heap? I guess you can put iter on stack.

wengxt · 2018-05-25T05:17:39Z

gui/src/Main.h

+public:
+    Q_PLUGIN_METADATA(IID "FcitxQtConfigUIFactoryInterface_iid" FILE "fcitx-rime-config.json")
+    explicit FcitxRimeConfigTool(QObject* parent = 0);
+    virtual QString name() override;


remove virtual.

Arfrever · 2018-05-30T03:46:11Z

Why does this patch use some parts of fcitx-qt (e.g.fcitxqtconfiguiwidget.h, fcitxqtkeysequencewidget.h, fcitxqtconfiguiplugin.h headers; fcitx-rime-config-gui is linked against PkgConfig::FcitxQt which theoretically is expanded into libfcitx-qt.so)?
fcitx-qt is for Qt 4.
libfcitx-qt.so library is linked against libQtCore.so.4, libQtDBus.so.4, libQtGui.so.4.
Using both Qt 4 and Qt 5 in the same process can cause misbehavior, crashes etc.
Defined symbols in Qt 4 libraries do not have versions.
Defined symbols in Qt 5 libraries use the same version Qt_5.
Qt-related undefined symbols in libfcitx-qt.so would be resolved by Qt 5 libraries.

Could you port to using Qt 5 only?

Actually libfcitx-rime-config-gui.so fails to build at linking stage:

[100%] Linking CXX shared module libfcitx-rime-config-gui.so
cd /var/tmp/portage/app-i18n/fcitx-rime-4.9999/work/fcitx-rime-4.9999_build/gui/src && /usr/bin/cmake -E cmake_link_script CMakeFiles/fcitx-rime-config-gui.dir/link.txt --verbose=1
/usr/bin/x86_64-pc-linux-gnu-g++ -fPIC -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter -fvisibility=hidden -march=native -O2 -fno-ident -frecord-gcc-switches -fstack-clash-protection -mfunction-return=thunk -mindirect-branch=thunk -mindirect-branch-register -pipe -Wall -Werror=terminate -Wl,--no-undefined -Wl,--as-needed -Wl,-O1 -Wl,--as-needed -Wl,--defsym=__gentoo_check_ldflags__=0 -Wl,--gc-sections -Wl,--sort-common -Wl,-z,now -shared  -o libfcitx-rime-config-gui.so CMakeFiles/fcitx-rime-config-gui.dir/Main.cpp.o CMakeFiles/fcitx-rime-config-gui.dir/ConfigMain.cpp.o CMakeFiles/fcitx-rime-config-gui.dir/Model.cpp.o CMakeFiles/fcitx-rime-config-gui.dir/FcitxRimeConfig.cpp.o CMakeFiles/fcitx-rime-config-gui.dir/fcitx-rime-config-gui_autogen/mocs_compilation.cpp.o -Wl,-rpath,/usr/lib64/qt4: /usr/lib64/libQt5Concurrent.so.5.11.0 /usr/lib64/libFcitxQt5WidgetsAddons.so.1.0 -lfcitx-utils -lfcitx-config -lfcitx-utils /usr/lib64/libQt5Widgets.so.5.11.0 /usr/lib64/libQt5Gui.so.5.11.0 /usr/lib64/libQt5Core.so.5.11.0 -lpkgcfg_lib_FcitxQt_fcitx-qt-NOTFOUND -lpkgcfg_lib_FcitxQt_fcitx-utils-NOTFOUND /usr/lib64/qt4/libQtDBus.so /usr/lib64/qt4/libQtXml.so /usr/lib64/qt4/libQtGui.so /usr/lib64/qt4/libQtCore.so -lrime -lfcitx-config 
/usr/lib/gcc/x86_64-pc-linux-gnu/8.1.1/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lpkgcfg_lib_FcitxQt_fcitx-qt-NOTFOUND
/usr/lib/gcc/x86_64-pc-linux-gnu/8.1.1/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lpkgcfg_lib_FcitxQt_fcitx-utils-NOTFOUND
collect2: error: ld returned 1 exit status
make[2]: *** [gui/src/CMakeFiles/fcitx-rime-config-gui.dir/build.make:209: gui/src/libfcitx-rime-config-gui.so] Error 1

xuzhao9 · 2018-05-30T16:39:57Z

I have removed the libfcitx-qt library from the build. See PR #41.

Arfrever · 2018-05-30T20:19:55Z

I confirm that building now works and libfcitx-qt.so is not used.

Arfrever · 2018-05-30T20:21:05Z

It would be nice if this GUI configuration tool was also ported to Fcitx 5 (https://github.com/fcitx/fcitx5-rime) :) .

wengxt suggested changes May 21, 2018

View reviewed changes

Added Rime GUI Config Tool support.

5c4a5f0

wengxt suggested changes May 24, 2018

View reviewed changes

[GUI] Remove subfolders and use inheritance for Ui class.

1d1a13a

wengxt suggested changes May 25, 2018

View reviewed changes

Move objects from heap to stack. Get rid of virtual keywords.

39f6d05

wengxt merged commit af3897b into fcitx:master May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GUI config tool support for fcitx-rime. #40

Add GUI config tool support for fcitx-rime. #40

xuzhao9 commented May 20, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 21, 2018

wengxt May 24, 2018

wengxt May 24, 2018

wengxt May 24, 2018

wengxt May 24, 2018

wengxt May 24, 2018

wengxt May 24, 2018

wengxt May 24, 2018

xuzhao9 May 24, 2018

wengxt May 24, 2018

wengxt May 24, 2018

wengxt May 25, 2018

wengxt May 25, 2018

wengxt May 25, 2018

Arfrever commented May 30, 2018 •

edited

Loading

xuzhao9 commented May 30, 2018

Arfrever commented May 30, 2018

Arfrever commented May 30, 2018



		// convert keyseq to state and sym
		FcitxKeySeq::FcitxKeySeq(const char* keyseq) {


		#include <rime_api.h>

		#include "fcitx-utils/keysym.h"

Add GUI config tool support for fcitx-rime. #40

Add GUI config tool support for fcitx-rime. #40

Conversation

xuzhao9 commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arfrever commented May 30, 2018 • edited Loading

xuzhao9 commented May 30, 2018

Arfrever commented May 30, 2018

Arfrever commented May 30, 2018

Arfrever commented May 30, 2018 •

edited

Loading