[Kde-pim] Review Request 125331: Port part of KCalUtils::IncidenceFormatter to Grantlee template system
Sandro Knauß
sknauss at kde.org
Wed Sep 30 12:36:19 BST 2015
> On Sept. 30, 2015, 11:05 vorm., Sandro Knauß wrote:
> > src/grantleetemplatemanager.cpp, line 80
> > <https://git.reviewboard.kde.org/r/125331/diff/3/?file=405098#file405098line80>
> >
> > Use a file template and use {% i18n "Template:" %}, that looks more clear for me.
> >
> > btw. you have now <b>Template::</b> is that intended :D
>
> Daniel Vrátil wrote:
> I don't think we can extract {% i18n %} from cpp files, that's why I opted for QString args.
that's why i would recommend to load this template by file.
> On Sept. 30, 2015, 11:05 vorm., Sandro Knauß wrote:
> > src/incidenceformatter.cpp, line 1071
> > <https://git.reviewboard.kde.org/r/125331/diff/3/?file=405100#file405100line1071>
> >
> > date2String should be done grantlee via:
> >
> > {{ date|date }}
> > {{ start|time }}
> >
> > and with grantlee we don't have to sperate date&time, because we can format it inside grantlee.
>
> Daniel Vrátil wrote:
> Unfortunately the logic in dateToString() and dateTimeToString() is bit more complicated (includes timezone resolution) than the Grantlee implementation.
>
> But I can create kdate, ktime and kdatetime filters in the Grantlee plugin and move the behaviour there, that could actually work :-)
sounds like a good plan.
> On Sept. 30, 2015, 11:05 vorm., Sandro Knauß wrote:
> > autotests/data/event-1.ical, line 7
> > <https://git.reviewboard.kde.org/r/125331/diff/4/?file=408871#file408871line7>
> >
> > please replace with a dummy address, or did you ask this guy?
>
> Daniel Vrátil wrote:
> I just stole this one from kcalcore testdata, so I assume he does not mind :)
:D
> On Sept. 30, 2015, 11:05 vorm., Sandro Knauß wrote:
> > autotests/data/event-2.html, line 75
> > <https://git.reviewboard.kde.org/r/125331/diff/4/?file=408872#file408872line75>
> >
> > these shoudn't be there (no nice html)
>
> Daniel Vrátil wrote:
> is perfectly valid and correct HTML, the idea is to prevent line breaks between image and string.
yes it is valid html, but it does not prevent line breaks - it only makes sure that the space is never removed, you only use it if you what to have a space between something, where none is. for image to text, you can use margins to have a nice spacing.
to prevent line breaks you should use nobr see (http://stackoverflow.com/questions/19212188/avoid-line-break-between-html-elements)
and still I don't get why we want exactly two spaces between name and emailaddress.
- Sandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125331/#review86149
-----------------------------------------------------------
On Sept. 30, 2015, 10:35 vorm., Daniel Vrátil wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125331/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 10:35 vorm.)
>
>
> Review request for KDEPIM, Laurent Montel and Volker Krause.
>
>
> Repository: kcalutils
>
>
> Description
> -------
>
> I needed to extend the HTML generated for todos so that it includes some more fields useful for the Phabricator resource. After digging around the code a bit a decided to see if I can port it to Grantlee, which is way easier to maintain and modify and also provides much cleaner separation between logic and representation and this is the result of it :)
>
> This patch does not port the entire IndicendeFormatter to Grantlee, only events, tasks, journals and free/busy, I haven't touched the ITIP formatter yet.
>
> The new code works just fine, but I consider it more of a proof-of-concept than a ready-to-ship patch and I'd like to get your input regarding the use of Grantlee - should we use a QObject wrapper for KCalCore::Incidence instead of QVariantHash, or maybe use the Grantlee MetaType system directly for thinner-than-QObject wrappers, should we use the custom Grantlee plugin, ...?
>
> Also please see the mail on kde-pim regarding where should we put the plugin :)
>
>
> Diffs
> -----
>
> CMakeLists.txt d145b6a
> src/grantleeki18nlocalizer_p.h PRE-CREATION
> src/grantlee_plugin/kcalendargrantleeplugin.cpp PRE-CREATION
> src/grantlee_plugin/icon.cpp PRE-CREATION
> src/CMakeLists.txt 0d7bdcc
> src/grantlee_plugin/CMakeLists.txt PRE-CREATION
> autotests/testincidenceformatter.h 0065745
> autotests/data/todo-1.html PRE-CREATION
> autotests/data/journal-1.html PRE-CREATION
> autotests/data/event-2.ical PRE-CREATION
> autotests/data/freebusy-1.html PRE-CREATION
> src/incidenceformatter.cpp c58e961
> templates/CMakeLists.txt PRE-CREATION
> templates/attendee_row.html PRE-CREATION
> templates/event.html PRE-CREATION
> templates/freebusy.html PRE-CREATION
> templates/incidence_header.html PRE-CREATION
> templates/journal.html PRE-CREATION
> templates/template_base.html PRE-CREATION
> templates/todo.html PRE-CREATION
> src/grantleetemplatemanager.cpp PRE-CREATION
> autotests/CMakeLists.txt a7ce2f1
> autotests/data/event-1.html PRE-CREATION
> autotests/data/event-2.html PRE-CREATION
> autotests/data/event-1.ical PRE-CREATION
> src/grantleetemplatemanager_p.h PRE-CREATION
> src/grantleeki18nlocalizer.cpp PRE-CREATION
> src/grantlee_plugin/kcalendargrantleeplugin.h PRE-CREATION
> src/grantlee_plugin/icon.h PRE-CREATION
> autotests/testincidenceformatter.cpp 28dd952
> autotests/data/todo-1.ical PRE-CREATION
> autotests/data/journal-1.ical PRE-CREATION
> autotests/data/freebusy-1.ical PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/125331/diff/
>
>
> Testing
> -------
>
> I can see my events.
>
>
> Thanks,
>
> Daniel Vrátil
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list