Emdek / plasmoid-adjustable-clock

Plasmoid to show date and time in adjustable format
GNU General Public License v2.0
20 stars 5 forks source link

The applet crashes plasma on settings change #1

Closed orivej closed 12 years ago

orivej commented 12 years ago

With KDE 4.8.0 and Adjustable Clock 3.0 on Funtoo Linux, Plasma Desktop crashes whenever I change time display format in the settings of the applet. The only time it does not crash seems to be when format is changed graphically and it does not span to the second line or further.

The same issue was present with all of the previous releases of Adjustable Clock that I've used before. Configured by editing the rc file directly the applet displays time just fine.

How am I supposed to debug this?

Emdek commented 12 years ago

I can't reproduce this, 4.8 on Kubuntu 32 bit. Have you recompiled it after upgrading to 4.8 (it is required due to libplasmaclock ABI changes)? Does after that show DrKonqui debug window with backtrace? Full one (with debug symbols installed for workspace and kdelibs) would be really useful (unless that it crashes only due to using version compiled on 4.7).

orivej commented 12 years ago

Here is the crash report: https://gist.github.com/1798259

I tried different sizes and placements of the panel, and the crash happens every time I try to apply new config in the settings of the applet. On the other hand, the crash never happens with a user account newly created (for testing).

Emdek commented 12 years ago

OK, it looks like race condition, thanks for backtrace. I'll try to add better protection to avoid it.

Emdek commented 12 years ago

Should be fixed now. Although currently other things can not work (due to configuration rewrite) you should backup you format first and in case of issues replace current code with 3.0 after testing.

orivej commented 12 years ago

Unfortunately it crashes. Crash report at the same URL.

Emdek commented 12 years ago

I see that this is the same issue (just in different place now), which shouldn't happen at all, since it looks like it tries to reach text edit when UI is during "destruction" phase (well, simply it's closing then)... You can try to add this code to Configuration.cpp:

    disconnect(m_appearanceUi.webView->page(), SIGNAL(contentsChanged()), this, SLOT(richTextChanged()));
    disconnect(m_appearanceUi.htmlTextEdit, SIGNAL(textChanged()), this, SLOT(sourceChanged()));
    disconnect(m_appearanceUi.cssTextEdit, SIGNAL(textChanged()), this, SLOT(sourceChanged()));
    disconnect(m_appearanceUi.backgroundButton, SIGNAL(clicked()), this, SLOT(backgroundChanged()));

Just after:

void Configuration::accepted()
{

This should be line 169.

orivej commented 12 years ago

After I inserted these lines, added the recompiled clock to the panel and applied a new format, Plasma crashed but DrKonqi didn't appear. Then I tried to apply a new format again and it worked. A couple of times then I deleted the clock, re-added it and reapplied the new format, and Plasma never crashed.

Emdek commented 12 years ago

So it is fixed now. :-) First crash probably comes from old instance already loaded.

Any other issues with new version (it is scheduled to be released on Wednesday)?

orivej commented 12 years ago

Great :) The only other issue is that the tooltip displays birthday events throughout the year.

Emdek commented 12 years ago

Ouch, I'll check code that is supposed to limit displayed events to only for next 12 or 24 hours.

orivej commented 12 years ago

And that with a dark desktop theme explanation text to the right of clock previews in the configuration is black on black.

Emdek commented 12 years ago

Thanks for report, I'll fix that.

orivej commented 12 years ago

And that “Apply” button in the configuration rests inactive regardless of changes on the first plane (e. g. when I select a different clock theme or edit its source string).

(Oops, Plasma just crashed when I canceled configuration changes in the applet, without restarting and without launching DrKonqi.)

Emdek commented 12 years ago

Apply is always dead in all applets using standard configuration UI (and AFAIR it can't be fixed). And that other crash is 99.9% the same issue again (but for another button). I've moved disconnecting to finished() handler, check current version.

orivej commented 12 years ago

OK, kdeinit restarts plasma-desktop with --nocrashhandler. I restarted it manually and pushed a report of the crash which happens when configuration is rejected in the settings to the gist, if need be.

Emdek commented 12 years ago

Check now (if you didn't check above fix yet), also I've fixed text color issue.

Emdek commented 12 years ago

And it's strange but I cannot reproduce that "all events visible" issue, at least with KOrganizer from 4.8.0, by creating two events, one for today, one for May (only today's shows up, as it should).

orivej commented 12 years ago

There are no crashes anymore!

I added events with KOrganizer as you did and the applet behaved properly. Nevertheless it displays all birthday events (added wiith KAddressBook).

Theme descriptions are now visible. However, the applet no longer displays anything (it only occupies some space according to the current theme).

orivej commented 12 years ago

Also note that the applet does not display any events upon plasma startup. I need to do something, like open its settings and press OK, or add another applet to the panel for it to display both birthdays and the event assigned for today. (A binary clock sitting aside displays events even when the adjustable clock does not.)

Emdek commented 12 years ago

Can you check if that theme for sure has HTML in source tab of editor?

I guess that data from KAddressBook has some differences from "standard" ones... I've tried to check that but after adding some data (including for today) nothing showed up... You can try to use plasmaengineexplorer to check what returns calendar data engine for query: events:2012-02-12:2012-02-15 Best way would be to maximize window then, resize value section and make screenshot (as AFAIK there is no better way currently).

Also it appears that events data is impossible to fetch just after creation of applet (data engine returns empty collection then), so I've changed it to update every minute (if needed - when clock or tool tip uses events placeholder), this will also help a bit with updating after changes are done (until I'll find better way to get notification about that). I'll try few more tricks to get it properly though.

Emdek commented 12 years ago

OK, I've found way to have events always synced, they should be displayed from start and their list will be updated immediately.

orivej commented 12 years ago

Yes, it had HTML. Now my own one has it too; and the applet in the panel is empty whereas a preview in the settings is correct. (But other themes disappeared with “Missing or invalid data file: …” instead of a preview.)

Yes, they seem to be different: "events:2012-02-12:2012-02-15" displays all birthday events; an example of one is uploaded to the same gist. (I just copied it with Ctrl+C. Dates were not displayed.) Yet when I added a birthday for today a binary clock somehow showed it alone.

Emdek commented 12 years ago

It looks kind like if data file couldn't be loaded but then there would be no entries on the list (except of custom themes)... Do those entries show description information on right (previews are cached so in such untypical cases like testing different versions they can show old data) or have HTML? Also best idea for testing just after installing (without restarting plasma-desktop) is to run it using command: plasmoidviewer adjustableclock

So if those are displayed for that range then this is good candidate for bugs.kde.org, as this is a bug in data engine or it's data source (maybe Akonadi?). But still it would be more usable if that viewer would be capable of displaying dates (now I don't know if they are there valid or not), I'm planning to check why this doesn't work there, and try to add proper export of data.

orivej commented 12 years ago

Nevermind, with latests commits the list reappeared…

Plasmoidviewer displays text in black, even though previews are already white (as they should be with my color theme). There is still nothing but an empty space in the panel (after restarting plasma-desktop, and re-adding the applet). Should I try to fix it myself (find exact offending commit etc.)?

New rich edit looks cool by the way! One problem: with "simple clock" theme deleting the colon between hours and minutes deletes them too.

OK, I'm going to fill a bug tomorrow.

Emdek commented 12 years ago

By text you mean descriptions or clock text? And you don't need to search for commit causing issues, I can reproduce this (it's panel only and I don't use it on panels...) and I have the only possible suspect. ;-)

I had even bigger plans for rich text editor, like allow to DnD parts but due to QtWebKit bug (tags with draggable attribute are... non draggable) and I'll apply workaround (custom DnD framework) for next release. I'm currently working on selection related issues right now (mostly to force selection of whole date / time parts), if it will be too buggy (but I would say that it already works better than previous solution...) then I'll revert it for current release.

Emdek commented 12 years ago

For that issue with panel try to replace: if (theme().background) { By: if (theme().background && formFactor() != Plasma::Horizontal && formFactor() != Plasma::Vertical) { In Applet.cpp, line 172. And restart plasma-desktop after installing.

orivej commented 12 years ago

The clock is visibile in a horizontal panel and in a vertical it is invisible. I have not tried it in the horizontal panel before the patch though.

Actually your clock was (and will be) great for me because of its adaptability to the vertical layout; my format string is %h:%m<br>%!$w<br>%!d<br>%!$M, and a year ago it looked like this: http://orivej.narod.ru/screenshots/20110203-kdm.png.

Emdek commented 12 years ago

It is supposed be adaptable. ;-) Thanks for testing, I'll do final test round before release. Now I'm still fighting with selection in rich text mode (to always force full placeholder selection, to avoid "bugs" - issues happening due to incorrect selection before applying style by user).