Submitting Kalendar for KDE Review

[Albert Astals Cid]

I think you may have dropped k-c-d from the CC, adding it back.


El dissabte, 16 d’octubre de 2021, a les 14:18:08 (CEST), Claudio Cambra va escriure:
> Hi Albert,
> 
> Thanks for your thorough review, and sorry about the late reply. We've been
> working on addressing these issues and have fixed them:
> 
> - Q_PROPERTY compilation warnings are now fixed
> - persistentIndex in the todomodel class are now QModelIndex
> - We now provide vendored copies of the components we require from Kirigami
> Addons (for now, until KA passes KDE Review)
> - We added KLocalizedString::setApplicationDomain("kalendar");
> - We now use standaloneMonthName for our month-year strings per your
> recommendation
> - We fixed the issue with valgrind you brought up
> 
> Additionally, we went ahead and addressed the user-facing issues you
> commented on (thanks!):
> - You should now get a pointing-hand cursor when hovering over links in the
> incidence information drawer
> - Clicking on empty space in the various views should now deselect the
> current incidence and close the incidence information drawer
> 
> Once again, thank you for taking the time to review Kalendar -- if you find
> anything else we should fix, we will be very happy to do so!

Good work!

Cheers,
  Albert

> 
> Clau
> 
> On Mon, Oct 11, 2021 at 12:01 AM Albert Astals Cid <aacid at kde.org> wrote:
> 
> > 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
> > >
> >
> >
> >
> >
> >
>