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

doc: Add "service" components and their launchables #115

Merged
merged 1 commit into from Jun 22, 2017

Conversation

mvollmer
Copy link
Contributor

No description provided.

@hughsie
Copy link
Collaborator

hughsie commented May 31, 2017

Out of interest, how do you see gnome-software display this type of component?

@mvollmer
Copy link
Contributor Author

mvollmer commented Jun 1, 2017

Out of interest, how do you see gnome-software display this type of component?

I was thinking that it would ignore it.

@hughsie
Copy link
Collaborator

hughsie commented Jun 1, 2017

Works for me! :)

Copy link
Owner

@ximion ximion left a comment

Choose a reason for hiding this comment

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

Looks good, but I have a few nits to pick ^^

Will you also add the C implementation? If not, that wouldn't be an issue as I am refactoring the parser anyway right now and the changes are really trivial (two new enum entries...)

</para>
<para>
If more than one <literal>launchable</literal> tag of type <literal>service</literal> is
present, a UI is expected to let the user manage all these services. Thus multiple
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: One excessive space here...

What does "manage" mean? Do you mean start? Do you mean start/restart/stop? What happens when the service is a "oneshot" unit? Maybe clarify that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to go into that much detail here? "Manage" should mean start/stop/restart/enable/disable/monitor, what ever the UI offers for a service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace fixed.

Copy link
Owner

Choose a reason for hiding this comment

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

I usually try to be precise here, because in case someone wants to implement a UI for a new software center but doesn't have the specific knowledge about services, it's helpful to know what the spec expects from UIs.
It also helps in case someone looks at the spec at a later date and wonders what the intentions were for a specific feature.

Arguably this is a rather nit-picky case here, as in case of Unix services it's well established what you can do with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would describe the UI of the launcher here, not of the software center, no? (For example, for desktop apps, we would be describing the GNOME Shell, not GNOME Software.)

I have tried to reword this without referring to "the UI" at all.

the Operating Systems "init" facility, such as systemd.
</para>
<para>
A service component is any software that is started and supervised by
Copy link
Owner

Choose a reason for hiding this comment

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

Duplicate text here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

the Operating Systems "init" facility, such as systemd.
</para>
<para>
The file described in this document is built upon the generic component metadata with fields specific for services (see <xref linkend="sect-Metadata-GenericComponent"/>).
Copy link
Owner

Choose a reason for hiding this comment

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

Don't say "file" as AppStream metadata might not be retrieved as file, and this generally is a bit confusing.
Usually, we use "component" here. Additionally, I try to avoid using the term "document" to refer to the text people are reading, because since there are XML documents and YAML documents as well as two types of AppStream "documents" this is confusing.
So, I'd probably have written something like "The component described here is built upon...."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other component types also use the term "file". Maybe we can change them all in a separate PR?

"Component" sounds wrong; we don't describe the component itself, but the format for expressing meta data about it. So what about "component metadata format"?

Copy link
Owner

Choose a reason for hiding this comment

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

Other component types also use the term "file". Maybe we can change them all in a separate PR?+

Jup, I can do that later.

"Component" sounds wrong; we don't describe the component itself, but the format for expressing meta data about it. So what about "component metadata format"?

You are right, "component metadata format" is way better - I think just writing "metadata" is fine as well (slightly less verbose)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

This tag is described in detail for generic components at <xref linkend="tag-provides"/>.
</para>
<para>
For services, at least one provided <code>&lt;service/&gt;</code> must be listed in this tag.
Copy link
Owner

Choose a reason for hiding this comment

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

Should it? And if so, what does the service provided-item contain as value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we don't need to mandate a <launchable> entry. Removed.

This tag is described in detail for generic components at <xref linkend="tag-launchable"/>.
</para>
<para>
At least one launchable element with type "service" must be present.
Copy link
Owner

Choose a reason for hiding this comment

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

Make that "service" a <literal>service</literal>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not relevant anymore since the text has been removed.

@mvollmer
Copy link
Contributor Author

Will you also add the C implementation?

I'd rather not.

@mvollmer
Copy link
Contributor Author

I have pushed a new commit with the fixes. Would you like me to squash them right away into the main commit?

@ximion
Copy link
Owner

ximion commented Jun 20, 2017

You can squash and force-push, if you want :-)
But ultimately with this PR it doesn't matter, as Github allows me to do a squash-and-merge when merging it, and the PR contains a single change that goes well in one commit.

(I replied to your replied on the inline comments)

@mvollmer
Copy link
Contributor Author

You can squash and force-push, if you want :-)

Okay, I'll do that!

ximion
ximion approved these changes Jun 22, 2017
@ximion
Copy link
Owner

ximion commented Jun 22, 2017

Looks good, thank you for your contribution!

I found some tiny indentation issues, which I'll resolve in some other commit later. Also, the C implementation is missing, but that's trivial and I'll add it for the next release. :-)

@ximion ximion merged commit 9c779f8 into ximion:master Jun 22, 2017
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