Skip to content

(very WIP) KF6 port #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Aug 23, 2024

At this point I’m basically just changing things until it compiles.

It does currently, but crashes with a “pure virtual method called” segfault on close, and I’m too bad with C++ to have an inkling about why that might be.

I’d be happy to help getting this into shape, but I’m also happy if you just take it as inspiration.

@ArchangeGabriel
Copy link

In #60 @alexfikl was saying they were wanting to port, a lot of code cleaning happened but I’m not sure where that porting effort lead to. Maybe there is some space for collaboration between the twos of you?

@alexfikl
Copy link
Contributor

alexfikl commented Nov 7, 2024

In #60 @alexfikl was saying they were wanting to port, a lot of code cleaning happened but I’m not sure where that porting effort lead to. Maybe there is some space for collaboration between the twos of you?

Unfortunately, I haven't had time to continue working on this after the general cleanups from a while back. My next item on the TODO list was porting to K_PLUGIN_CLASS_WITH_JSON, which doesn't seem included in here and I think the old K_PLUGIN_FACTORY was removed in KF6 (?).

I'm also not sure if it's desired to keep the codebase working on both Qt5 and Qt6 (before it was working on both Qt4 and Qt5). It definitely requires some discussion and planning 😁

@arthurzam
Copy link

Hi, could we have some reply from the maintainer of the application?

@fhackenberger
Copy link
Owner

Original author here. I'm not sure if @jfmcarreira is still active, he was maintaining ktikz mostly. Maybe wait a few days to give him a chance to reply. If he doesn't, I'd say if you are up for moving forward @alexfikl feel free to. Supporting two versions of Qt is nice, but given the maturity of the application, I'd argue its fine to only support the latest version for new releases, if that makes development easier. Security fixes can then still be backported. If you need repository permissions, just shoot me an email. Thank you for your contributions!

@jfmcarreira
Copy link
Collaborator

I would agree that it might be easy moving foward to keep only Qt6 support. I believe the application is mature enough and not seen new features appearing that it is deal breaker to keep the current version if you dont have Qt6 available.

Sorry for not following along. Nowadays, I do not use ktikz as often as I used to, and not using Linux on my daily driver is hard to keep mantaining it.

@jfmcarreira
Copy link
Collaborator

@flying-sheep Hello could you please remove the vscode project files from the PR.

I will try to test and give you some feedback on the PR

Copy link
Contributor

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good already! 😁

Things that seem still left to do:

  • I can confirm that it gives a segmentation fault on exit (and maybe some other situations), but haven't had the time to look at it.
  • The CI will need some massaging to get the qt6 dependencies installed correctly. Need help with that?

@@ -9,6 +9,10 @@ endif()
set(KTIKZ_VERSION "0.13.2")
set(KTIKZ_USE_KTEXTEDITOR TRUE CACHE BOOL "Use KTextEditor framework")

set(QT_MIN_VERSION "6.5.0")
set(KF6_DEP_VERSION "6.0.0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This KF6_DEP_VERSION should be used in find_package(ECM ...) below as in the current file for konsole:
https://github.com/KDE/konsole/blob/a4dfe903cd920978828a91ecfce2c8055fc5fa15/CMakeLists.txt

Comment on lines -35 to +41
Qt5
5.15
CONFIG
REQUIRED Core Gui Widgets Xml PrintSupport LinguistTools
Qt6
REQUIRED
COMPONENTS Core Gui Widgets Xml PrintSupport LinguistTools Core5Compat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the QT_MIN_VERSION variable be used here? Again, the CMakeLists.txt for konsole is probably a good inspiration. :D

Comment on lines +45 to +47
KF6
REQUIRED
COMPONENTS DocTools XmlGui TextEditor Parts IconThemes KIO WidgetsAddons I18n
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also use KF6_DEP_VERSION here?

Comment on lines -64 to 70
// const QRegExp expression(rule.pattern);
// int index = text.indexOf(expression);
QRegExp expression(rule.pattern);
int index = expression.indexIn(text);
while (index >= 0) {
const int length = expression.matchedLength();
auto m = rule.pattern.match(text);
while (m.hasMatch()) {
int index = m.capturedStart();
const int length = m.capturedLength();
setFormat(index, length, rule.format);
// index = text.indexOf(expression, index + length);
index = expression.indexIn(text, index + length);
m = rule.pattern.match(text, m.capturedEnd());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could maybe work better with global matching?
https://doc.qt.io/qt-6/qregularexpression.html#global-matching

setCurrentEncoding(in.codec());
//setCurrentEncoding(in.codec());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This codec stuff probably needs fixing before merging. I'm not sure what the current way of working is..

KTextEditor::CodeCompletionInterface *cci =
qobject_cast<KTextEditor::CodeCompletionInterface *>(m_documentView);
if (cci) {
m_completion = new TikzKTextEditorCompletion(this);
cci->registerCompletionModel(m_completion);
}
m_completion = new TikzKTextEditorCompletion(this);
m_documentView->registerCompletionModel(m_completion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is at fault, but I tried writing some code and completion doesn't seem to be working.

)

install(TARGETS ktikzpart DESTINATION ${KDE_INSTALL_PLUGINDIR})
install(FILES ktikzpart.rc DESTINATION ${KTIKZPART_DATA_INSTALL_DIR})
install(FILES ktikzpart.desktop DESTINATION ${KDE_INSTALL_KSERVICES5DIR})
#install(FILES ktikzpart.desktop DESTINATION ${KDE_INSTALL_KSERVICES5DIR})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably just be removed. The json metadata gets compiled into the library, as far as I know.

@alexfikl
Copy link
Contributor

Original author here. I'm not sure if @jfmcarreira is still active, he was maintaining ktikz mostly. Maybe wait a few days to give him a chance to reply. If he doesn't, I'd say if you are up for moving forward @alexfikl feel free to. Supporting two versions of Qt is nice, but given the maturity of the application, I'd argue its fine to only support the latest version for new releases, if that makes development easier. Security fixes can then still be backported. If you need repository permissions, just shoot me an email. Thank you for your contributions!

Thank you for the kind words! I still use ktikz quite often, so I would very much like to see it stick around (thus my initial burst last year to get it closer to Qt6). I'm not a C++/Qt developer though, so I'm not very confident I can help much with maintaining / fixing bugs :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants