Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tklockinterface: add tklock property for rerunning when LPS is shown #1

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

teleshoes
Copy link
Contributor

one feature at a time so you can critique. next feature will be adding a billboard dbus iface to trigger a rerun {or perhaps arbitrary prop setting, e.g.: qdbus io.thp.billboard / setProp "klomp-song" "Rinse the Raindrops"}

@@ -234,6 +237,8 @@ class Daemon : public QObject {

tmpl["battery-bar"] = "{{=" + QString::number((double)(tmpl["battery"].toDouble()*.01)) + "}}";

tmpl["tklock"] = tklock.isLocked() ? "locked" : "unlocked";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could just be:

tmpl["tklock"] = tklock.getLockMode();

Where the implementation of TklockInterface has:

QString lockMode() { return m_lock_mode; }

@thp
Copy link
Member

thp commented Mar 15, 2015

Added some comments - please have a look and if possible update the commit (--amend) and force-push so that we can merge this change with the fixes as single commit. Thanks!

@thp
Copy link
Member

thp commented Mar 15, 2015

If we just want to update the content unconditionally every time the screen is locked (I think that's okay), then you can also remove the tklock property (i.e. being able to use {tklock}), as it won't be useful to the user -- just use the tklock locked() signal for contextChanged() (unconditionally).

* Billboard - Low Power Mode Standby Screen for the N9
* Webpage: http://thp.io/2012/billboard/
* Copyright (C) 2012, 2013, 2014 Thomas Perl <thp.io/about>
* Copyright 2015 Elliot Wolk <elliot.wolk@gmail.com>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please note the modified copyright header on the NEW files i added. dunno what youd prefer. i dont have any particular preferences {leave it like this, modify it to make it clear that this small thing is the only thing i did, put your original header back and just add an authors/contributors, whatever}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to just have your line, as I didn't do anything in this file (yet). Also use "Copyright (C) 2015" (note the " (C)") in order to be consistent with the rest of the copyright notices.

@teleshoes
Copy link
Contributor Author

whoops, missed that you asked for a single commit, lemme rebase

@teleshoes
Copy link
Contributor Author

im going to add the if-isLocked to contextChanged(), and remove it if you dont like it

@teleshoes teleshoes force-pushed the feature_tklock branch 2 times, most recently from 563cd95 to b24fb60 Compare March 15, 2015 17:06
@@ -282,7 +288,9 @@ class Daemon : public QObject {
}

void contextChanged() {
updateTimer.start();
if (tklock.isLocked()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't check for isLocked() here - it will break the preview in the GUI application (the screen is never locked when the user uses the GUI application).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, it looks like it ought to just call render. it doesnt break the preview it for me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a more subtle issue - if Billboard is started in locked state (see other comment), then isLocked() will never become true until the screen is unlocked and locked again. So please remove it (here and the function altogether) and let's just use the locked() signal for doing an additional contextChanged().

Also, when the screen isn't locked, updating the Billboard isn't all that costly, as the screen is on and the CPU is probably running, as the user is interacting with the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, ill do it now

@thp
Copy link
Member

thp commented Mar 20, 2015

You can ignore my previous comments about combining locked() and unlocked() - leave the locked() and unlocked() signals as they are, they actually make sense now that I think of it (and the way we're connecting only to the locked() signal - avoids us having to filter the events in the signal handler, and just connect it directly to contextChanged(), which is nice).

thp added a commit that referenced this pull request Mar 20, 2015
tklockinterface: add tklock property for rerunning when LPS is shown
@thp thp merged commit be45baf into harmattan:master Mar 20, 2015
@thp
Copy link
Member

thp commented Mar 20, 2015

Merged, thanks!

@teleshoes
Copy link
Contributor Author

yay, thanks!

@teleshoes teleshoes deleted the feature_tklock branch March 20, 2015 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants