libreliodev / android

Android Customizable Reader - Librelio Digital Publishing Suite
www.librelio.com
226 stars 130 forks source link

Replace application_.xml by Application_.plist #242

Closed libreliodev closed 10 years ago

libreliodev commented 10 years ago

After introducing the new tab feature (#153), we are now in a situation which may seem confusing:

Additionally, similar settings have different names on iOS (see https://github.com/libreliodev/iOS/wiki/Building) and Android.

Last but not least, when new settings are added, we need to update all application_.xml files of published apps otherwise we cannot build the apps anymore.

It would make more sense to remove application.xml, and to replace it by an Application.plist similar to the one we have on iOS.

libreliodev commented 10 years ago

@intrications

So, should I basically take the plist from the iOS app and make that work on Android?

Yes, as documented on https://github.com/libreliodev/iOS/wiki/Building . However, some settings are not useful on Android (such as SharedSecret), but it should not matter.

We will also need to add the following Android settings

 <string name="yearly_subs_code">yearlysubscription</string>
<string name="monthly_subs_code">monthlysubscriptiontest</string>
<string name="gcm_project_number">1012641315687</string>

Please change the name with camel case, as we do on iOS. All other settings are already in the iOS plist, or could be derived from the package name, as we do on iOS. The idea is to have as few settings as possible.

Please make sure missing settings in the plist do not crash the app.

intrications commented 10 years ago

@libreliodev Is the test magazine functionality still required?

https://github.com/libreliodev/android/blob/master/src/com/librelio/activity/StartupActivity.java#L315

intrications commented 10 years ago

@libreliodev How do you want to deal with this in the Android app: AppId: the ID on the App Store. If this key is present, the "Rate This App" feature will be activated, if the key is omitted, it will not

The app id would equate to the package name. But this can be determined in code, it doesn't need to be in the plist.

libreliodev commented 10 years ago

@intrications

Is the test magazine functionality still required?

No it's no longer needed. After completing #245, we should be able to add a pdf to the Assets directory for testing, without the need to download it from the server.

libreliodev commented 10 years ago

@intrications

How do you want to deal with this in the Android app:

AppId: the ID on the App Store. If this key is present, the "Rate This App" feature will be activated, if the key is omitted, it will not

The app id would equate to the package name. But this can be determined in code, it doesn't need to be in the plist.

I think the best option is to do as follows:

intrications commented 10 years ago

@libreliodev

No it's no longer needed. After completing #245, we should be able to add a pdf to the Assets directory for testing, without the need to download it from the server.

It seems MuPDF can't open a PDF directly from the assets folder. Assets don't appear as Files, they are only accessible as InputStreams. So this won't work. Should I retain the test magazine code?

libreliodev commented 10 years ago

@intrications

It seems MuPDF can't open a PDF directly from the assets folder. Assets don't appear as Files, they are only accessible as InputStreams. So this won't work. Should I retain the test magazine code?

OK, thanks, we'll see how we can embed pdf files in the app later. Anyway, you can remove the test magazine code.

intrications commented 10 years ago

@libreliodev

The code just pushed takes some of its settings from Application_.plist.

Unfortunately, there is no official code for setting the Google Analytics tracking ID from code so that will probably have to stay in the xml. It may be possible to do with with Google Analytics v3 but I'm not sure - we're currently on v2.

Are there any settings in https://github.com/libreliodev/android/blob/master/res/values/application_.xml that are no longer required? Any other comments before I switch them to Application_.plist?

libreliodev commented 10 years ago

Unfortunately, there is no official code for setting the Google Analytics tracking ID from code so that will probably have to stay in the xml. It may be possible to do with with Google Analytics v3 but I'm not sure - we're currently on v2.

I am embarrassed, because keeping both application.xml et Application.plist will still make things more confusing, I think. Any other suggestions?

Alternatively, we could keep application_.xml, using the same names (when relavant) as on iOS. But in this case, is there a way to make it possible to build the app even when a setting is missing in the xml?

intrications commented 10 years ago

@libreliodev

Options:

a) Continue to try using Plist and upgrade Google Analytics to v3 or v4. There are some suggestions on the internet about setting the tracking ID in code but it is unclear which version they are for and whether they do work.

However, Google Analytics should probably be updated to v3 or v4 anyway to get any bug fixes. Tracked in #247.


Options b) and c) below give two ways of using XML but improving it so it could be customized easier and allow missing XML resources. Really XML is probably better. It is the built in Android way and the XML is compiled into binary in the APK so it is more efficient.

b) Switch to building with Gradle. By using product flavors we could customize very easily. I'm sure this could still work with your bash script because Gradle can be called from the command line in the same way as the previous Android build tools.

c) Switch the app to a library project and then create another project that references that library which overrides the XML references.

Both b) and c) would allow us to complete #243 also.

I might not have explained it well enough here to make a choice.

Of course, c) will probably be the preferred option because that wouldn't require any changes to your bash script and would be relatively quick.

libreliodev commented 10 years ago

@intrications

Thanks for clarifying these options.

I don't think c) can be chosen at this time, for consistency reasons. The different versions of our apps are packaged as "Customizable apps", not libraries. We need to keep it this way for now.

We might choose b) providing we can still build using our current methods: ant or eclipse. I am not sure I express myself correctly here, but you get the idea. Gradle should come as an additional option.

Regarding a), I agree that anyway we should upgrade the GA library. Now, you wrote that "Really, XML is probably better", which makes a lot of sense.

Now, I have 2 questions:

1- One of the original reasons why we wanted to change was that we had a plist for tabs, and an xml file for other settings, which was confusing. Could tabs.plist be moved to xml? Would it be a problem, considering that we can have a variable number of tabs, in a variable order ...?

2- If I understood well, moving Tabs.plist to xml would solve #242, without needing #245, correct?

intrications commented 10 years ago

@libreliodev c) would still create a customizable app. It would actually be more customisable because any XML file could be overwritten. I think I will have to create a test project for you to show you. It would be quick to do.

Gradle replaces ant and works much better in Android Studio than Eclipse so let's forget that again for now. But long term Gradle is the future for Android builds and is much more reliable already, in my opinion.

  1. Not so simple with XML to have a variable number of tabs. Might be able to use arrays. But it may look more messy than the Plist.
  2. 245 is done. And I don't think XML would fix #242. If using XML, then there would be no way to override the version in the APK.

I actually think Plist is better for Tabs, partly because it is already done and working and the same Plist can generally be used on iOS and Android. A lot of the settings in application_.xml are specific to Android so it's not so bad it not being a Plist.

libreliodev commented 10 years ago

@intrications

OK, let's keep tabs in plist, and settings in xml. Let's just clean application_.xml, and remove redundant/unneeded elements.

Thanks!

intrications commented 10 years ago

@libreliodev Ok, I've reverted the changes. Let's start by cleaning out application_.xml. Which of these are no longer required?

<string name="app_name">Wind</string>
<string name="root_view">Magazines.plist?waupdate=30</string>
<string name="yearly_subs_code">yearlysubscription</string>
<string name="monthly_subs_code">monthlysubscriptiontest</string>
<bool name="enable_test_magazine">false</bool>
<bool name="enable_code_subs">true</bool>
<bool name="enable_username_password_login">true</bool>
<string name="client_name" >niveales</string>
<string name="magazine_name">wind</string>
<string name="service_name">biwing</string>
<string name="google_analytics">UA-1732003-23</string>
<bool name="enable_send_log_in_menu">false</bool>
<bool name="enable_downloads_in_menu">true</bool>
<bool name="enable_refresh_in_menu">true</bool>
<bool name="enable_subscribe_in_menu">false</bool>
<bool name="enable_info_in_menu">false</bool>
<string name="info_url">http://www.example.com</string>
<bool name="enable_app_rating">true</bool>
<string name="gcm_project_number">1012641315687</string>
libreliodev commented 10 years ago

@intrications Here are the ones I believe are no longer required. Please let me know if you think I am wrong

intrications commented 10 years ago
<bool name="enable_code_subs">true</bool> replace by code_service
<bool name="enable_username_password_login">true</bool>redundant with service name (if service name is not empty, this should be true)

So both code and username/password login are active if code_service is not empty? What is the code_service for Wind?

<string name="client_name" >niveales</string>should be deducted from package
<string name="magazine_name">wind</string>should be deducted from package

Just to confirm, the package name always takes that form of com.niveales.wind where the last part is magazine_name and the second to last part is the client name?

<bool name="enable_send_log_in_menu">false</bool> will be in tabs

I presume this isn't implemented yet. Is it tracked in a github issue?

libreliodev commented 10 years ago

So both code and username/password login are active if code_service is not empty? What is the code_service for Wind?

We have services for checking either single codes (name=code_service) or username/password combinations (currently name=service, could be replaced by name=user_service to clarify). Normally, it's either one or the other (or none). For wind, the values should be:

 <string name="code_service" >S3</string>
 <string name="user_service" ></string>

Just to confirm, the package name always takes that form of com.niveales.wind where the last part is magazine_name and the second to last part is the client name?

Yes. In come cases it might be something like br.org.client.app, so it's important take last and second to last, not second and third.

<bool name="enable_send_log_in_menu">false</bool> will be in tabs

I presume this isn't implemented yet. Is it tracked in a github issue?

Sorry, this was a mistake, this can simply be deleted.

intrications commented 10 years ago

The current service_name is used in this line to add a parameter to the username/password query:

https://github.com/libreliodev/android/blob/master/src/com/librelio/activity/BillingActivity.java#L616

Is the old service_name equal to the new user_service?

code_service is not currently used in the code - should it be?

libreliodev commented 10 years ago

The current service_name is used in this line to add a parameter to the username/password query: https://github.com/libreliodev/android/blob/master/src/com/librelio/activity/BillingActivity.java#L616 Is the old service_name equal to the new user_service?

Yes, correct.

code_service is not currently used in the code - should it be?

It's not used because we just have one code_service for now: S3. But it might be used in the future.

intrications commented 10 years ago

Ok, I've made all those changes.

Any other changes should probably be in new issues since this is about switching to a Plist.

libreliodev commented 10 years ago

OK, will test tomorrow. Thanks.

libreliodev commented 10 years ago

@intrications Sorry, I forgot to mention 2 elements that I think should be deleted, don't you think so?

<bool name="enable_info_in_menu">false</bool>
<string name="info_url">http://xxxxxxxx</string>
intrications commented 10 years ago

@libreliodev Agreed.

libreliodev commented 10 years ago

Thanks.