Submitting Kalendar for KDE Review

[Albert Astals Cid]

El diumenge, 10 d’octubre de 2021, a les 22:45:06 (CEST), Claudio Cambra va escriure:
> Hey all,
> 
> We would like to move Kalendar to KDE review. We would also like to ask if
> it would be possible to be release along with KDE Gear since the KDE
> PIM libraries don't have a stable ABI, and we need to make sure we
> remain compatible with them.
> 
> For those who don't know, Kalendar is a new calendar application built
> with Kirigami and Akonadi. Our repo is located at
> https://invent.kde.org/pim/kalendar/. We welcome any feedback!

These seem like important enough to fix

AutoMoc: /home/tsdgeos/devel/kde/kalendar/src/hourlyincidencemodel.h:30: Warning: Property declaration model has no READ accessor function or associated MEMBER variable. The property will be invalid.
AutoMoc: /home/tsdgeos/devel/kde/kalendar/src/incidenceoccurrencemodel.h:42: Warning: Property declaration filter has no READ accessor function or associated MEMBER variable. The property will be invalid.
AutoMoc: /home/tsdgeos/devel/kde/kalendar/src/multidayincidencemodel.h:35: Warning: Property declaration model has no READ accessor function or associated MEMBER variable. The property will be invalid.
AutoMoc: /home/tsdgeos/devel/kde/kalendar/src/todosortfilterproxymodel.h:17: Warning: Property declaration incidenceChanger has no READ accessor function or associated MEMBER variable. The property will be invalid.
/home/tsdgeos/devel/kde/kalendar/src/todosortfilterproxymodel.h:18: Warning: Property declaration calendar has no READ accessor function or associated MEMBER variable. The property will be invalid.
/home/tsdgeos/devel/kde/kalendar/src/todosortfilterproxymodel.h:19: Warning: Property declaration filter has no READ accessor function or associated MEMBER variable. The property will be invalid.




This is also interesting

/home/tsdgeos/devel/kde/kalendar/src/todomodel.cpp:168:39: warning: loop variable ‘persistentIndex’ of type ‘const QPersistentModelIndex&’ binds to a temporary constructed from type ‘const QModelIndex’ [-Wrange-loop-construct]
  168 |     for (const QPersistentModelIndex &persistentIndex : persistentIndexes) {
        m_persistentIndexes << persistentIndex; // Stuff we have to update onLayoutChanged


You're pretending to keep a list of persistent model indexes, but you're not, because m_persistentIndexes is a QModelIndexList that is a QList<QModelIndex> so you're losing the "persistence" of it.

So you either 

  A) don't need the persistance of it all if this works and persistentIndex should just be a QModelIndex
or
  B) you should make m_persistentIndexes a list of QPersistentModelIndex  (and remove the & from the persistentIndex declaration to make the warning go away).



Aaaaaaaaaaaaaand I can't run it ^_^

QQmlApplicationEngine failed to load component
qrc:/main.qml:663:9: Type TodoPage unavailable
qrc:/TodoPage.qml:168:17: Type TodoTreeView unavailable
qrc:/TodoTreeView.qml:9:1: module "org.kde.kirigamiaddons.treeview" is not installed

What provides org.kde.kirigamiaddons.treeview ?
Ok kirigami-addons, this is a bit "problematic" because it is still listed under unstable in our download section https://download.kde.org/unstable/kirigami-addons/0.2/
Having something stable depend on something unstable is not "good".



You're missing a KLocalizedString::setApplicationDomain("kalendar"); just after your QApplication


I get a
"0 instead of 1 arguments to message {Ends on %1} supplied before conversion."
when selecting one of my existing events


You can't use "<b>MMMM</b> yyyy" to get month and year because for some languages "MMMM" is translated as "of monthname" so that the typical DD MMMM yyyy looks fine for them and you get "25 of Janaury of 2023", but then if you do "MMMM yyyy" you get "of monthname 2021" which is wrong, so my suggestion is do something like i18nc("%1 is month name, %2 is year", "%1 %2", Locale.standaloneMonthName(bla), blo));



Running with valgrind i got
==42992== Invalid read of size 16
==42992==    at 0x142A6C69: ???
==42992==    by 0x191F6C17: ???
==42992==  Address 0x191f6c1e is 30 bytes inside a block of size 42 alloc'd
==42992==    at 0x483E7C5: malloc (vg_replace_malloc.c:380)
==42992==    by 0x7B2E7D2: QArrayData::allocate(unsigned long, unsigned long, unsigned long, QFlags<QArrayData::AllocationOption>) (in /usr/lib/libQt5Core.so.5.15.2)
==42992==    by 0x7BA967A: QString::fromLatin1_helper(char const*, int) (in /usr/lib/libQt5Core.so.5.15.2)
==42992==    by 0x1C68FE: QString::QString(QLatin1String) (qstring.h:1067)
==42992==    by 0x1F1886: AttendeeStatusModel::AttendeeStatusModel(QObject*) (attendeesmodel.cpp:33)
==42992==    by 0x1F223C: AttendeesModel::AttendeesModel(QObject*, QSharedPointer<KCalendarCore::Incidence>) (attendeesmodel.cpp:78)
==42992==    by 0x1E2F2E: IncidenceWrapper::IncidenceWrapper(QObject*) (incidencewrapper.cpp:15)
==42992==    by 0x1BC3E8: QQmlPrivate::QQmlElement<IncidenceWrapper>::QQmlElement() (qqmlprivate.h:139)
==42992==    by 0x1BC435: void QQmlPrivate::createInto<IncidenceWrapper>(void*) (qqmlprivate.h:166)
==42992==    by 0x5674D62: QQmlType::create(QObject**, void**, unsigned long) const (in /usr/lib/libQt5Qml.so.5.15.2)
==42992==    by 0x56C6F1D: QQmlObjectCreator::createInstance(int, QObject*, bool) (in /usr/lib/libQt5Qml.so.5.15.2)
==42992==    by 0x56C7675: QQmlObjectCreator::create(int, QObject*, QQmlInstantiationInterrupt*, int) (in /usr/lib/libQt5Qml.so.5.15.2)

When opening an event



Now "as a user" comments, feel free to ignore:
 * It feels weird that the urls on the description field of my events is clickable but i get no "pointy-hand" mouse icon on hovering over them
 * Same for the attendees to a meeting
 * I need a "add default reminder" option
 * It's weird that you can't unselect events clicking on empty space

Good start!

Cheers,
  Albert

> 
> Thanks,
> Clau
>