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

Microsoft AutoUpdate #3

Merged
merged 4 commits into from Jul 20, 2018

Conversation

Projects
None yet
2 participants
@clburlison
Contributor

clburlison commented Jul 20, 2018

This adds the com.microsoft.autoupdate2 manifest. All keys are working properly and the output profile is correct. With that said the Applications dictionary is kind of ugly? It works but has a bunch of subkeys since it's a dictionary of dictionaries.

There is also a tiny issue with the pfm_value_placeholder and supporting integer values, reported at erikberglund/ProfileCreator#50. With that said I don't feel it's a blocker on merging this if you want.

Also ran this repo through a spell checker and fixed a few strings.

Cheers, Clayton

clburlison added some commits Jul 20, 2018

feat: Add MAU manifest
All the keys are working but the Applications dict is kind of ugly from
a visual standpoint.
@erikberglund

This comment has been minimized.

Owner

erikberglund commented Jul 20, 2018

Thank you! This is great. I've looked at it and it works as you say.

One thing that was interesting was that in a few of places there were duplicate <key> tags, that I didn't think would work, but the app reads those as strings. That was something new for me. I mean:

<key>pfm_title</key>
<key>Microsoft Excel</key>

Instead of:

<key>pfm_title</key>
<string>Microsoft Excel</string>

I saw the placeholder value issue, and have looked at it and it accepts any value, but the specific number text field that displays the value was wrong, I have updated that.

Thanks for the spelling check, that was needed.

And lastly, about the Application dictionary. There are a few options, I saw you wrote in the description that the values should not be changed? Is that true, so what actually should be included is the dict with those static values? Is that correct. In that case one can add some keys and make it look like this instead. See that for excel you include "Excel" and not the values. That will get both included at the same time. And the OneNote is the way it was before:

screen shot 2018-07-20 at 16 26 02

Just wanted to ask.

I think it would also be possible to hide the values so you only see the application to include but that assumes that the values should be static?

Anyway, the version in the screenshot includes the values but they are not static and can be changed, but they are both required to be included. If that is the case then this would be a good change so you can't do something wrong, or is it possible to only include one of the values?

@clburlison

This comment has been minimized.

Contributor

clburlison commented Jul 20, 2018

<key>pfm_title</key>
<key>Microsoft Excel</key>

Ha this is actually pretty funny and kind of surprised ProfileCreator didn't error on this. Will fix in a followup commit.

Application dictionary.

There are a few options, I saw you wrote in the description that the values should not be changed?

Yes, these are static for all users of Office 2016 and shouldn't be changed.

Is that true, so what actually should be included is the dict with those static values? Is that correct.

Correct

In that case one can add some keys and make it look like this instead. See that for excel you include "Excel" and not the values.

The change you made here is perfect!

I think it would also be possible to hide the values so you only see the application to include but that assumes that the values should be static?

If we can hide the Application ID and LCID for all of the sub-dictionary values I think that is absolutely the way to go.

If that is the case then this would be a good change so you can't do something wrong, or is it possible to only include one of the values?

No both keys are required. It would be best if the Application ID and LCID keys are immutable however I couldn't find an easy way yesterday in my quick searchs. If we set the correct value and hide the text input boxes I think that is the overall best solution. Visually speaking end users shouldn't have to worry about those two keys since they are simply required and shouldn't be changed.

@clburlison

This comment has been minimized.

Contributor

clburlison commented Jul 20, 2018

Well after sleep I now found the hide option. So the only thing left is to add the Excel change you made. I'm not sure how you made Excel clickable so I'll need some help on that piece.

@erikberglund

This comment has been minimized.

Owner

erikberglund commented Jul 20, 2018

Then you add:

<key>pfm_require</key>
<string>always</string>

or

<key>pfm_required</key>
<true/>

(they mean the same thing)

To both the AppID and LCID.

Why it's not clickable is to make things more clear, and that I haven't coded the option to click the container and include all options in the next level below. So then I hide the +. But, if a key is required, then there is no + beside that, which makes it so you have to select the container, and then only the required keys will be added automatically.

I will probably look at that some more to maybe add the option to include all optional keys when including the container.

Anyway, if you add both pfm_hidden and required then it will be a list of applications.

But we could improve this a little more possibly, not in the manifest right now but for the next release if it's needed. But we'll start with this version to get it going.

@clburlison

This comment has been minimized.

Contributor

clburlison commented Jul 20, 2018

That makes sense. Thanks for explaining.

👍 on merging.

@erikberglund erikberglund merged commit ea4ec4e into erikberglund:master Jul 20, 2018

@erikberglund

This comment has been minimized.

Owner

erikberglund commented Jul 20, 2018

I added a small change now, and thought I tell you. THere is a key for number textfields that is often missed: pfm_value_unit that will let you specify the unit of the value and that will be added after the textfield to make it more obvious what you are entering.

So, in this case i added:

<key>pfm_value_unit</key>
<string>Minutes</string>

to the UpdateCheckFrequency key to have that show together with the text field to make it super obvious that it's minutes that you are entering there.

@erikberglund

This comment has been minimized.

Owner

erikberglund commented Jul 20, 2018

Ah, and another thing. Lol. This is funny, I found a slight bug in the placeholder string for some views, and thought that was what was affecting you, but I just tried to see if that fixed it, and found that you had this key in the dict:

<key>pfm_default</key>
<true/>

And default means the default value, and true is 1, so that is why you were seeing the 1 instead of 720. If I removed default, it worked as expected on beta 3 as well.

I saw that on many dicts in your manifest, and that might have been a copy-paste artifact?

Atleast I fixed a bug for another view!

@erikberglund

This comment has been minimized.

Owner

erikberglund commented Jul 20, 2018

Sorry for the many posts, the HowToCheck key had 0 as default, but it needs a string from the pfm_range_list to actually set the default value, else it just sets the first value.

What should the default be for that key?

Manual
AutomaticCheck
AutomaticDownload

@clburlison clburlison deleted the clburlison:mau branch Jul 20, 2018

@clburlison

This comment has been minimized.

Contributor

clburlison commented Jul 20, 2018

The minute addition is 👍

The pfm_defaults being boolean definitely was a copy/paste error.

Lastly, the HowToCheck key should default to AutomaticCheck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment