Re: KJournald in KDE-Review
Hi Kai, many thanks for this very detailed review! For the nitpicks, I did a first cleanup. Some of the not low hanging fruits regarding UI and features I added to the backlog items in the Invent project. More details below. Cheers, Andreas On Donnerstag, 3. November 2022 14:58:06 CET Kai Uwe Broulik wrote: > * bootmodel.cpp: I don't think you need to call beginResetModel() when > you populate it in the constructor done > * bootmodel.cpp: QAbstractListModel is a flat list with a single column, > no need to re-implement parent(), columnCount(), etc done > * bootmodel.cpp: Admittedly it's prettier to have an Q_INVOKABLE for > bootId but if you made the roles Q_ENUM, you could call data() from QML > wiht e.g. bootModel.data(bootModel.index(row, 0), BootModel.FooRole) thanks, was actually an obsolete method, but Q_ENUM for roles is obviously a good thing > * colorizer.cpp: Multi-look-up: !contains() + operator[] + value(), > probably use find() and friends done > * fieldfilterproxymodel.cpp: Could use the enum values/ints instead of > custom mapping through QString (c.f. Q_ENUM suggestion above) I am ambivalent about this. Yes, it makes sense to provide an enum-based filtering but then from the API POV I am restricting the API to fields that I know, yet journald allows to use individual fields names without them being defined in any specification. For now, I will keep to strings and probably add a convenience API for very well known fields. > * fieldfilterproxymodel.h: you can probably READ rowCount for > Q_PROPERTY(int count) since it has a default-argument, instead of adding > a count() that just calls rowCount() thanks, done > * fieldfilterproxymodel.h: get() seems unused? yes, it currently is, will keep it in mind before making the API public to maybe remove it > * fieldfilterproxymodel.cpp: It feels like your filterAcceptsRow() impl > is superfluous and you could be using setFilterFixedString? nice cleanup, thanks > * filtercriteriamodel.cpp: You should emit dataChanged() in setData() > when data has changed. It already is in FilterCriteriaModel::setData(...), you were probably looking at SelectionEntry::setData(...) which is an internal data class but not exposed > * journaldhelper.cpp: instead of going QString -> std::string -> c_str, > you could be doing qUtf8Printable() Nice! That method was new to me. > * journaldhelper.cpp: mapField could be using QMetaEnum to turn an enum > into a string Good idea! done > * journalduniquequerymodel.cpp: openJournalFromPath() could be reusing > the QFileInfo instance for isDir() and isFile() check done > * journalduniquequerymodel.cpp: openJournalFromPath() returns true, even > if opening fails Ouch, thanks! > * journalduniquequerymodel.cpp: it wasn't entirely clear to me what the > std::pair<.., bool> is for You find a quite unclean code fragment that needs refactoring ;) Will put it on my list. Essentially, the boolean is reused for a selected property, which is quite wrong to put here... > * journalduniquequerymodel.cpp: you probably shouldn't return true if > setData() didn't change anything? Done > * journalduniquequerymodel.cpp: selectedEntries() appears unused Nice, already one user of the above hack, that I could remove :) > > I suggest you run QAbstractItemModelTester against your models to verify > they fully behave as Qt expects. Actually, that is already in place in all autotests, just double-checked... > Some comments on the UI: > * Would be nice to have a filter bar in the "Unit" and "Process" list I > have a thousand units there, it's hard to find them. added to this issue https://invent.kde.org/system/kjournald/-/issues/22 > * There should be tooltips on elided items, e.g. when the Unit name gets > elided in the left side filter area, there are tooltips, do you mean others? > * the ItemDelegate in the bootIdComboBox should have width: parent.width done > * The scrolling behavior for the journal content is awful, you probably > want to wrap your ListView in a ScrollView for proper desktop-y mouse > wheel scrolling the scrolling itself is not the problem here, but rather the caching logic, which is needed to interact with very big logs (I have 1-2 GB logs for single boots e.g. that I have to scroll efficiently) but there are improvements possible... added to this issue https://invent.kde.org/system/kjournald/-/issues/6 > * There isn't really any keyboard navigation, e.g. I should be able to > focus the LogView and use arrow keys to go up and down an item try the page up/down keys ;) but yes, this needs to be worked on, added to this issue https://invent.kde.org/system/kjournald/-/issues/6 > * A filter feature rather than highlight would be lovely, e.g. "kde" and > find all items matching kde added to this issue https://invent.kde.org/system/kjournald/-/issues/23 > * It's not really obvious that selecting copies to clipboard, the > selection should stay, and maybe there should be a context menu added to this issue
Re: KJournald in KDE-Review
On Sonntag, 23. Oktober 2022 10:34:36 CET Andreas Cord-Landwehr wrote: > Hi, just a short update: Thank you for the comments! Nearly all of them are > already fixed. For a very few I created invent.k.o issues instead, because > they are mostly a maintainability concern and make sense to fix together > with planned features (e.g. removal of fixed colors when refactoring > integration with desktop color scheme). > If you have more comments please raise them. In case nobody finds a blocker, > I will ask sysadmins at the end of the week to move kjournald to the > "system" module. Hi, kjournald now transitioned into the system module. Thanks to all! Andreas
Re: KJournald in KDE-Review
Hi, just a short update: Thank you for the comments! Nearly all of them are already fixed. For a very few I created invent.k.o issues instead, because they are mostly a maintainability concern and make sense to fix together with planned features (e.g. removal of fixed colors when refactoring integration with desktop color scheme). If you have more comments please raise them. In case nobody finds a blocker, I will ask sysadmins at the end of the week to move kjournald to the "system" module. Cheers, Andreas
Re: KJournald in KDE-Review
On Mittwoch, 12. Oktober 2022 03:56:03 CEST Aleix Pol wrote: [...] > Then I'd recommend to only make it public API when the time comes. :) > > +1 to making it optional instead, if it makes your life any better. > That said, when we've needed this kind of flexibility in the past, we > have often ended up splitting the repository into a separate one, not > sure it would make sense here. > > Also it would be useful to know what your plans are. :D Hi, just added such an option. My goal is to eventually split the library part out from the application as soon as it makes sense. At the moment there are still sometimes changes to the library API/moving parts that are not fully polished yet. The full story is that I started with the library, then noticed that my reference app to show how the API can be used has a lot of value as its own. So the focus right now is to make something for end users and later on to look at finalizing the library abstraction. Cheers, Andreas
Re: KJournald in KDE-Review
On Montag, 10. Oktober 2022 15:34:30 CEST Aleix Pol wrote: > On Sun, Oct 9, 2022 at 7:18 PM Andreas Cord-Landwehr [...] > > Even though KJournald is currently contained in the "libraries" playground > > module, I would like to get it included in the "utilities" module after > > passing KDE Review. The reason is that at the moment I more focus on the > > application part and that is the most user-facing part. Having it in > > "utilities" thus will avoid confusion. > > > > Looking forward for your review comments! > > I'm a bit confused by the context. > > It's placed in libraries although it seems like system would be the > place for it. > https://invent.kde.org/system > > I see there's a library, are you planning on maintaining compatibility > there? Are there other users? If there's no users outside, it could > make sense to skip installing headers until there is a use-case, > otherwise you might see yourself tied to an ABI unnecessarily. Yes, the https://invent.kde.org/system module sounds actually to be the best place for the app; I wrote "utilities" in the original mail, but agree that system is even better. I myself have use cases to use the library part as a library. Yet that is not for a public project. I could introduce a build option to only install headers optionally (default being OFF), which might makes it more clear to packagers that currently they do not have to package the headers. Moreover, at the moment their is not yet a fully stable API for the library, which is probably another good reason to not install headers by default. In the mid-term/long-run, I am planning to provide a stable API for the library. Cheers, Andreas
KJournald in KDE-Review
Hi, after a few releases over the past year, I would like to get KJournald included in KDE application releases. This project is about providing both an QItemModel abstraction library for the C-style journald API and providing an efficient graphical browser for journald logs. Sysadmins moved the repository to KDE Review today, you can find the source code here: https://invent.kde.org/libraries/kjournald (flatpack packaging is also available for easy trying it out) Even though KJournald is currently contained in the "libraries" playground module, I would like to get it included in the "utilities" module after passing KDE Review. The reason is that at the moment I more focus on the application part and that is the most user-facing part. Having it in "utilities" thus will avoid confusion. Looking forward for your review comments! Best regards, Andreas
Re: KDE Frameworks 6 - Virtual Sprint setup
On Sonntag, 31. Januar 2021 16:04:32 CET Christoph Cullmann wrote: > On 2021-01-30 12:14, Volker Krause wrote: > > On Freitag, 29. Januar 2021 15:57:59 CET Adam Szopa wrote: > >> Hello, > >> I've been talking with David Faure about setting up a Sprint focused > >> on KF6 > >> work. Some of the topics would include: > >> - Reviewing the KF6 board > >> (https://phabricator.kde.org/project/board/310/[1]): -- Clean up > >> -- Tagging Junior Jobs > >> - Working out a structure/process for handling: > >> -- "Stuck" tasks > >> -- Unit test regressions > >> - Decide the 5.15 minimum requirement bump timeline > >> - Decide on a 6 branching strategy and timeline > >> - Decide if/how ECM should support multiple Qt versions > >> > >> That's just an example list - the wiki[1] should include the up to > >> date and > >> detailed topics. > >> > >> The Sprint will use the KDE BBB instance - same tech that powered last > >> years > >> > >> Akademy; I'll coordinate that with our sysadmins to have it > >> > >> available. > >> > >> As for the date and length, usually Virtual Sprints are a weekend > >> thing. I'd > >> love to hear from you if that sounds OK, and which weekend would be > >> workable for you (how soon can we get this started) and your preferred > >> availability hours. I'll set up a poll later to pinpoint the final > >> timing. > > > > Thanks for driving this, I'm in! European hours preferred, any weekend > > starting from w6 should be possible. > > Count me in, too ;=) > European hours preferred. Same for me! Cheers, Andreas
Re: CI testing with local files
On Donnerstag, 28. Januar 2021 10:20:54 CET Konstantin Kharlamov wrote: > On Wed, 2021-01-27 at 17:36 -0500, Michael Reeves wrote: > > Is there way to reading and writing small files in the CI? I'm in the > > process of writing auto tests for kdiff3. > > Your tests could create a directory inside the current one, and then inside > the new directory do stuff like creating files, etc. So, unless I > misunderstand your question, there's that. Hi, best practise is to put those files into a QTemporaryDir or create them as QTemporaryFile to avoid contamination from a previous test run. Also you should not change input data during a test that will be needed unchanged for another test run but prefer copying it to a temporary location. Best regards, Andreas
Re: Review Request 128749: Backport karchive fix for out of directory files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128749/#review98647 --- Ship it! Yeah, thanks applying it! LGTM, only point is that I cannot see the test-tar archive in the diff, but I presume that reviewboard cannot display binary files - Andreas Cord-Landwehr On Aug. 24, 2016, 10:36 nachm., Albert Astals Cid wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128749/ > --- > > (Updated Aug. 24, 2016, 10:36 nachm.) > > > Review request for kdelibs, Andreas Cord-Landwehr and David Faure. > > > Repository: kdelibs > > > Description > --- > > What the summary says > > > Diffs > - > > kdecore/io/karchive.cpp eb0bf2e > kdecore/tests/karchivetest.h e64e0ed > kdecore/tests/karchivetest.cpp 7786b98 > > Diff: https://git.reviewboard.kde.org/r/128749/diff/ > > > Testing > --- > > > Thanks, > > Albert Astals Cid > >
Re: Minuet (music education software) moved to kdereview
On Friday, January 29, 2016 6:53:23 PM CET Sandro Andrade wrote: > Question: Minuet uses TiMidity++ and freepats as run-time > dependencies. Should I improve CMakeLists.txt to detect such > installations (they aren't libraries, maybe some ugly check is > required) as a way to hint packagers to add those as Minuet's > dependencies? Hi, if they are really only runtime dependencies, a README.packagers that clearly states such dependencies is the better way to go. The rationale behind this is that for packages the build-process is usually decoupled from running applications (at least for binary distributions). The building is done on different systems than those the compiled software will be executed. So, a CMake check would be misleading, since such dependencies are not needed for building. Moreover they have to be specifically setup as runtime-dependencies to be automatically installed for the users. Cheers, Andreas
Re: Re: Minuet (music education software) moved to kdereview
On Monday 25 January 2016 21:48:27 Albert Astals Cid wrote: > El Sunday 24 January 2016, a les 16:50:18, Andreas Cord-Landwehr va escriure: > > * it looks strange to me that in minuet/cmake/ there are Config-files for > > the 3rd-party library drumstick. My understanding was that such Config > > files should only be shipped with the respective library (maybe someone > > with a deeper CMake-knowledge can comment?) > > If upstream ships a cmake file awesome, but if not then we have to find it > having our own cmake file. Absolutely, but shouldn't that be find-files then instead of config-files? I always had the perception that those are quite different. Cheers, Andreas
Re: Minuet (music education software) moved to kdereview
Hi Sandro, it is always great when such a cool application lands in KDE Edu. I just made a first and rough (since I do not have all dependencies yet to really compile and test it) code review. Here a some minors I noticed: * the application does not link against KCrash (which is needed for DrKonqi); also in the main.cpp there should be a call to "KCrash::initialize();" * an appdata file is missing * in the main CMakeLists.txt, for KF5 there is no minimum version and for ECM the minimum version should probably be higher than 1.0.0 * it looks strange to me that in minuet/cmake/ there are Config-files for the 3rd-party library drumstick. My understanding was that such Config files should only be shipped with the respective library (maybe someone with a deeper CMake-knowledge can comment?) * qml-file-localization: It looks strange to me that you are mixing two different localization systems with KI18n (in all C++ and UI files) and qsTr in all QML files. Since I am not familiar with qsTr, I cannot really comment on how it works and if everything is setup correctly; at least Messages.sh do not process qml files and hence do not extract strings from them, but that might only be necessary for Ki18n? Other than that, the code quality and licensing information look really good. Cheers, Andreas
Re: Minuet (music education software) moved to kdereview
On Sunday, January 24, 2016 6:47:17 PM CET Elvis Angelaccio wrote: > 2016-01-24 16:50 GMT+01:00 Andreas Cord-Landwehr <cordlandw...@kde.org>: > > also in the main.cpp there should be a call to "KCrash::initialize();" > > Shouldn't this happen automatically? > This is what the documentation at [1] seems to imply, at least. Hi, in non-distro cases it happens automatically. Yet, many distro packaging systems (discussed in this thread [1]) optimize such linking out, if there is no explicit call to the library. That's actual the reason why nowadays there exists KCrash::initialize(). Cheers, Andreas [1] https://mail.kde.org/pipermail/kde-frameworks-devel/2015-September/ 027029.html
Re: Re: Re: KDE Applications Versioning
On Tuesday 14 July 2015 07:08:57 Andreas Cord-Landwehr wrote: If it is OK this way, I can add it later today to the wiki page. Hi, since I did not hear any oppositions, I just added the paragraph to the wiki page draft. Cheers, Andreas
Re: Re: KDE Applications Versioning
On Monday 13 July 2015 23:14:17 Albert Astals Cid wrote: I did a few tweaks, i still feel it seems this is the official way other than an optional way of defining the version but maybe that's just me. Hi, I have the same feeling as Albert that the current text is not clear enough that both variants (increase version by hand and using the scripted approach) are fine. What about adding an introduction paragraph like the following: - snip - Every application has an application version number that regular has to be increased to distinguish different versions of the application (e.g., features, bugfixes). When an application does not have its own release schedule but is released alongside with KDE Applications, there is also the version number of the KDE Applications release. It is the maintainer's duty to take care on increasing the version number regularly and there are specifically two possible ways: 1. increase the version number by hand for each new release 2. use the same version number as KDE Applications and let the release script update the version number In the following, we explain how to use the scripted version numbers. - snap - If it is OK this way, I can add it later today to the wiki page. Cheers, Andreas
Re: Scripting/Extensions BoF at Akademy?
Nice idea to do a BoF about this topic! Since we use a lot of QtScript in Rocs for scripting (and I really would like to also offer Python support), I am also quite interested. Do you want to suggest a time slot for the BoF? cheers, Andreas
Re: KDE Review - Moving Artikulate to KDE Edu
On Sunday 26 January 2014 16:39:08 Albert Astals Cid wrote: El Dimarts, 14 de gener de 2014, a les 19:52:57, Albert Astals Cid va So I did have a look at Artikulate yesterday and just found some minor nagging user flow issues that I told Andreas and he says he would be fixing. Yeah, all fixed now. Other than that, looks good to me to move to kdeedu *but* we need to decide what to do with kqmlgraphplugin first, since we can't have a kdeedu app depending on a playground lib. Since kqtquickcharts is in KDE Review since the 5th, if no big issues are found it should migrate at the 19th to KDE Edu. So this problem will be solved 7 days before the freeze :) Greetings, Andreas
Re: Re: kqmlgraphplugin -- a QML plugin to render beautiful and interactive graphs
On Tuesday 28 January 2014 23:26:58 Albert Astals Cid wrote: I think the code is pretty stable. It's used inside KTouch without any problems since KDE 4.10. I'm less sure about how the release should be done and there these components fits in. Should this plugin be distributed on its own or as part of something bigger? Good, question, at this stage i'd either put it inside the kde-edu virtual module and get it released if with 4.13 and on or put it in extragear-libs and you care of releasing it yourself. Hi Sebastian, so do you want to/can you put kqtquickcharts into KDE-Review with the goal to have it in Extragear at the time of KDE SC 4.13? Since the time in Extragear will be at least two weeks (according to the KDE application lifecycle) it would be great to do that soon to have it in place at the time of the SC freeze. (hence to allow Artikulate to migrate to SC in time) Any possible API cleanups (if any are necessary?) then could happen with the migration to Qt5/KF5 I think. Greetings, Andreas
Re: KDE Review - Moving Artikulate to KDE Edu
On Sunday 26 January 2014 17:24:11 Albert Astals Cid wrote: Other than that, looks good to me to move to kdeedu *but* we need to decide what to do with kqmlgraphplugin first, since we can't have a kdeedu app depending on a playground lib. Are there technical reasons why we cannot? You can't compile something against something that is unreleased. Hi, would it be enough to have a release of kqmlgraphplugin even if it is still in playground? (i.e. released tarballs at download.kde.org) I am also putting Sebastian into CC, as he maintains kqmlgraphplugin Greetings, Andreas PS: to be precise kqmlgraphlpugin is a runtime-dependency, not a buildtime one
Re: Re: KDE Review - Moving Artikulate to KDE Edu
Hi, I think you should work on getting kqmlgraphplugin released first, depending on something unreleased scares me a bit. Just talked to Sebastian (maintainer of the plugin) and he will do a technical preview release of the plugin soon. BTW this code already exists in KTouch for quite some time and now is only split into a self-contained plugin. After compiling and installing kqmlgraphplugin and artikulate i got artikulate(10361) LearnerProfile::Storage::database: Database path: /home/kdeunstable/.kde/share/apps/artikulate/learnerdata.db artikulate(10361) LearnerProfile::ProfileManagerPrivate::ProfileManagerPrivate: No last active profile found, falling back to first found profile artikulate(10361): There is no subdirectory skeletons in directory . cannot load skeletons. artikulate(10361): There is no subdirectory courses in directory . cannot load courses. When running it and no window at all. As discussed on IRC, the program starts now (was access problem to the sound card making Phonon to wait infinitely.) Are the course downloads working now or do you still have a problem with the KNS dialog? (if not, we should move that discussion to a bug report probably) Greetings, Andreas
KDE Review - Moving Artikulate to KDE Edu
Hey all, after about one year of development and two technical preview releases, we thought that is time to move Artikulate to KDE Review, with the goal to land eventually in the KDE Educational module. (a heads-up mail about this proposal also goes to the KDE-Edu list) The repository for review is available here: https://projects.kde.org/projects/kdereview/artikulate The remaining of this mail is structured in: 1. What is Artikulate and why should it belong to KDE Edu 2. Compile, build and install instructions 3. The bigger picture about Artikulate (condider this part first for skipping, if mail is too long :) = What is Artikulate = Artikulate is pronuncation training application that works by repeated listening to native speaker recordings, recording of the learner's own voice, and comparing both to see if the currently learned phrase/pronunciation is already mastered or not. The application itself consists of two parts: 1. the learner part: aimed for the common learner explained in the Handbook (docbook format, see /doc folder) 2. the editor part: aimed for contributors who create courses currently we only aim to have technical course contributors and hence its documentation is available only at http://techbase.kde.org/Projects/Edu/Artikulate = Compile, build, and install = We have extended documentation about how to build and install Artikulate, to be found in the techbase wiki: http://techbase.kde.org/Projects/Edu/Artikulate/BuildAndInstall At this point, I only want to give two remarks: * when running, please make sure that the QML_IMPORT_PATH is correctly set (as explained at the wiki page), since otherwise the application closes with a fatal error * we require the (yet) unreleased kqmlgraphplugin for plotting statistics graphs; this library/qml plugin is available in the KDE playground and its QML files also have to be included in the QML_IMPORT_PATH (details at the wiki page) = The bigger picture = Artikulate shall close the gap for pronunciation training applications in KDE Edu, since the focus of the currently available applications only lies on vocabulary training and memorizing (also mixed with edutainment applications). The idea for this application isn't new, as there are a few commercial (but AFAIK barely known) Windows applications with similar learning concepts. Interestingly, those existing commercial applications are very hard to purchase. As nearly every educational application, also for Artikulate it is crucial to have a set of high quality learning courses. For the beginning, we went the way to focus on /scenario courses/. This is, we collected phrases typical for scenarios one could face in real life (e.g. ordering food in a restaurant, asking for the way,...) By this we were able to create a blueprint for further courses that can simply be created by translating all phrases and recording them by native speakers. At the moment, essentially the courses for Bengali and Polish are done and there is active work on a course for Greek. The focus of our work for this year will hence be on recruting further contributors to finish scenario courses for more languages (and especially core languages like English, French, Spanish). Distribution of these courses is done over the downloads.kde.org infrastructure using knewstuff. (The next iteration of this will probably go the Bodega way. Another focus -- once entered into KDE SC and hence once the application is available easier for non- technical contributors -- a major goal is to start a cooperation with language teachers for 1-2 core languages. The current state of the application is, that we think all essential features are done by now and we need to increase our user/tester base to get more feedback. That's essentially the point why we would like to get into the SC soon, to iron old the remaining rough edges and check early before hard-freeze that everything works on a wide range on platforms/distributions. Greetings, Andreas
