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

[lipstick] enablers for devicelock pass code age #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rainisto
Copy link
Contributor

devicelock encryption plugin is introducing new return values (exit 2 and exit 3) for special cases like lockcode is found in history file (stores xx values that config key states) and for lockcode has expired (older than xx days). This change changes boolean call to return int (but keeping the interoperability with for example jolla-home package with the kludge in lines 201-203).

@giucam
Copy link
Contributor

giucam commented Sep 19, 2014

Can't it return an enum instead of a generic int with no defined meaning?

@rainisto
Copy link
Contributor Author

This is called from qml which doesn't have support for enum's as such. So I need to use int in qml side anyways. I tought that it might be clearer to have int also in c++ side.

And its also more future proof, so if devicelock encryption plugin introduces new return value, then I wouldnt have to create new pull request to lipstick.

But yes with enum's c++ side might be a bit more readable.

@giucam
Copy link
Contributor

giucam commented Sep 25, 2014

This is called from qml which doesn't have support for enum's as such

What do you mean? You can have enums in QML.

@rainisto
Copy link
Contributor Author

Please tell more? http://qt-project.org/doc/qt-5/qml-enumeration.html#using-the-enumeration-type-in-qml at least states that enums are vars or ints.

@giucam
Copy link
Contributor

giucam commented Sep 25, 2014

Mmh.. ok, but i was thinking that you could still define an enum with the values returned by the plugin.

@pvuorela
Copy link
Contributor

Concur that it would be nice having some named values instead of magic numbers somewhere. This could be the place since it's already swapping return codes 0 and 1.

@rainisto
Copy link
Contributor Author

Sure if it makes code more readable I can add named values. I'll make an update to pull request tomorrow or so.

@rainisto
Copy link
Contributor Author

Added enum.

@faenil
Copy link
Member

faenil commented Sep 26, 2014

still using "return 0" though :)

@rainisto
Copy link
Contributor Author

well yes still using return value trickery, because I dont want to lock people out of their devices who using Jolla devel when they update lipstick only. But enum should now 'ducument' the current possible return values from the plugin.

@giucam
Copy link
Contributor

giucam commented Sep 26, 2014

What do you mean? Failed is 0, so returning that, or using it to compare it against a value is safe.
I gather checkValue and setValue are meant to be called from QML too, right? If so, i'd declare Q_ENUMS(ReturnValue).

@rainisto
Copy link
Contributor Author

Q_ENUMS(ReturnValue) is already declared in line 33 in .h file. It is returning failed as 0 to QML. (Binary itself returns 1 on failed and 0 on success).

Ah sorry my crosseyes didnt notice return 0 lines... fixing.

@rainisto
Copy link
Contributor Author

Now should be better.

@giucam
Copy link
Contributor

giucam commented Sep 29, 2014

LGTM

@rainisto
Copy link
Contributor Author

Updated pull request with qmlRegisterUncreatableType so that Q_ENUM is also exported in qml side so for example jolla-home can access enum values.


class MGConfItem;
class QTimer;

class DeviceLock : public QObject, protected QDBusContext
class LIPSTICK_EXPORT DeviceLock : public QObject, protected QDBusContext
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the LIPSTICK_EXPORT necessary for the qmlRegisterUncreatableType call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, without the export there was undefined symbol when trying to use the library.

@giucam
Copy link
Contributor

giucam commented Nov 14, 2014

Ok, looks good.

@@ -199,7 +199,9 @@ bool DeviceLock::runPlugin(const QStringList &args)
qWarning() << process.readAllStandardError();
#endif

return process.exitCode() == 0;
if (process.exitCode() == 0) return OK;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the enum, so that OK is 0 and Failed is 1, you don't need do have that thing here, and can just say return process.exitCode();

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't - that would mean if (!runPlugin(...)) { /* plugin returned success */ }, which seems more error-prone.

Copy link
Contributor

Choose a reason for hiding this comment

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

..although, since there are positive error codes that also mean failure now, you might be right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current enum order is there because of keeping compatibility with jolla-home and settings-system. That way people don't get locked out of their devices if they only update lipstick and somehow forget to update jolla-home and settings-system at the same time. (as those projects have hard coded in some places that OK == 1), so it would be safer to keep compatibility and gradually create pull request to switch hardcoded values to use the enum.

And anyways this pull request is currently on hold as devicelock rewrite study might change the way how the pass code age is handled.

@thp
Copy link
Contributor

thp commented Jan 29, 2015

I guess this is on hold for now, keeping it open?

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.

6 participants