ditek / MidiSheetMusic-Android

Play and visualize MIDI music files
GNU General Public License v3.0
130 stars 51 forks source link

update to support Android 14 #27

Open bmx666 opened 1 year ago

bmx666 commented 1 year ago

Hi @ditek , I finished migration on the latest Android Studio Flamingo to support your app on Android 12 and below. I faced with an issue on Android 13 to support Scoped Storage. I am still working on it.

Do you plan to maintain this repository and publish the app on F-Droid?

ditek commented 1 year ago

Hi and thanks for contributing. I'm not actively developing new features, but PRs are definitely welcome! Others have mentioned publishing to F-Droid before. I'm not really familiar with the publishing process. I'll have to look it up and see what needs to be done.

Once you're done let me know so I could do some testing. It would also be helpful if you mention what device/emulator you are testing on.

bmx666 commented 1 year ago

Hi @ditek , I will make rework to support API33 and above and run few tests to check if there're no a side effects after update. Also I can make a template for F-Droid, I already published one of my app. I noticed there's your app was published a long time ago but it could be a problem related to sign process. Do you still have a sign key for this app?

bmx666 commented 1 year ago

I made quick tests and didn't notice any critical issues. I found one bug related to external storage permission, if permission was revoked when FileBrowser tab is still displaying content then app will crush. And that's why I would like to move away from checking and requesting external storage permission on startup in splash screen and request it only in FileBrowser tab. I wanna run and check app, and I don't need to allow app access external storage because it already has internal MIDI samples. Anyway, the major goal will be nice to migrate on Kotlin that gives more features and cleaner code.

ditek commented 1 year ago

Also I can make a template for F-Droid, I already published one of my app. I noticed there's your app was published a long time ago but it could be a problem related to sign process. Do you still have a sign key for this app?

The one published is from an old version before I took over the project, so I don't have the sign key for that and therefore I will be publishing this as a new app. I'll try to look into it in the weekend.

I wanna run and check app, and I don't need to allow app access external storage because it already has internal MIDI samples.

Sounds like a good idea. But can we do that in a separate PR? Let's try to keep this one just for compatibility with latest Android Studio.

Anyway, the major goal will be nice to migrate on Kotlin that gives more features and cleaner code.

I have started using Kotlin for all new files, but still use Java for changes on existing files. I feel that the amount of work migrating and potential for breaking things is not really worth it.

bmx666 commented 1 year ago

Hi @ditek , thanks for quick response.

The one published is from an old version before I took over the project, so I don't have the sign key for that and therefore I will be publishing this as a new app. I'll try to look into it in the weekend.

I made a quick fastline template for F-Droid in my repo. I took short and full description from original MidiSheetMusic app from F-Droid and mix with README from your project, but it's up to you.

NOTE: short description must not exceed 80 characters.

If you want to pass all steps of publishing, I can save your time and give tips directly:

  1. You can read about Fastline here -> Fastlane Cheat Sheet implemented by @IzzySoft , you can find some info also in my PR.
  2. It would be nice if you generate a signed key and upload it directly to this git repository so that it is easy to maintain in the future 😇
  3. Try to build release APK then tag it, tag should follow by versionName like v1.0.0 -> "1.0.0", to keep manifest on F-Droid repo without additional changes.
  4. Fastline changelog file names must correspond to the releases versionCode, literally no padding, so if the versionCode is 1, the file name should be 1.txt.
  5. Then visit Requests For Packaging and create a new issue, I think it will be nice also add some notice about old published app on F-Droid, maybe it could be removed from F-Droid.
  6. Depends on workload you will get label - ready after 1 week or 2 months.
  7. Maintainer can then create a merge request in F-Droid data, but I recommend doing this yourself, otherwise it may take weeks or months. For example, there is my first MR, but a new auto-generated version of the yaml file can be found here.

Good luck! 🥳🥳🥳

ditek commented 1 year ago

Great! Thanks for your input. I'll look into it 😊

bmx666 commented 1 year ago

Today I also verified MIDI input on my phone with Android 13 after update and it works!

IzzySoft commented 1 year ago

As I was pinged, let me amend step 7 with a link to my own short instructions :wink:

bmx666 commented 1 year ago

Hehe, looks like original author Madhav Vaidy made a new version on Google Play recently - https://play.google.com/store/apps/details?id=com.midisheetmusic

ditek commented 1 year ago

Interesting. That's a good thing. The more open source projects out there the better 🙂 BTW, have you pushed all the changes to this PR so I can test it? If so, let me know which phone you tested on and its specs (resolution, etc.)

bmx666 commented 1 year ago

The more open source projects out there the better 🙂

seems original app will stay Freeware, I didn't find any new source code that had published since 2013, but anyway it's better to have ad-free app at least 😃

TW, have you pushed all the changes to this PR so I can test it?

Yes, no more updates from my side. And I left my branch "f-droid" as separated.

which phone you tested on and its specs (resolution, etc.)

I tested on all emulators from 21 up to 34, based on Pixel 2 template 1080x1920, 420dpi and Google Play/Google API arm64 images. BTW, I found one issue on API 23 and below, when the App tries to display a huge MIDI file then it crashes, some error related to out-of-memory 😞 But I didn't notice that problem at least on Android 8 and later, but you can check it. And I tested my MIDI keyboard with Sony Xperia 1 III, stock Android 13, I didn't notice any issues.

Ah, and another little annoying bug, switching left/right notes sheet doesn't resize properly, but after scrolling it disappears.

Anyway, thank you for this project, my goal was to create MIDI keyboard trainer app, based on your app. Like these, but open-source:

And the rest of apps on Google Play and F-Droid don't support MIDI keyboard, and it's really strange to pay 5-10$ for them 🙅

ditek commented 1 year ago

Sorry for keeping you waiting. I'm struggling a bit with health issues at the moment. Upon testing the app I found a few issues.

How it looks in the PR:

How it looks on Master:

All of the tests were performed on a Pixel 3a API 34 emulator.

I also have some comments on the code. I'll try to add them as soon as possible.

ditek commented 1 year ago

Ah, and another little annoying bug, switching left/right notes sheet doesn't resize properly, but after scrolling it disappears.

I have noticed several visual glitches, but haven't gotten the chance to look into them. It would be great if you could document them and add them as issues.

Anyway, thank you for this project, my goal was to create MIDI keyboard trainer app, based on your app. Like these, but open-source

You're welcome. I've had similar ideas as well like adding a training mode that one can toggle. Even in the paid apps, I didn't find one that would allow you to add your own sheets. Probably for intellectual property reasons.

bmx666 commented 1 year ago

Hi @ditek , no worries, take your time, health is more important ❤️

After freshly installing the app you get asked for file access permission. There is not way to get out of this screen if you don't accept to grant the permission and the back button won't work until you grant it.

If you grant the permission and enter the app, the back button doesn't work to get out of the app. It works how ever if you are playing a song and want to get back to the main screen.

Yes, I already mentioned about that (https://github.com/ditek/MidiSheetMusic-Android/pull/27#issuecomment-1613767156) and you asked about new PR for that (https://github.com/ditek/MidiSheetMusic-Android/pull/27#issuecomment-1615139972). The File Browser tab should request permission for that.

The settings side-menu now takes the color of the system theme, i.e. if the system theme is light, the menu will be white. This breaks app appearance consistency and show always be dark regardless of the system theme.

I think it relates to migrate on materialdrawer v9.0.1 and add SettingsTheme to support DayNight theme for API33 and above otherwise devices on Android 13+ has some issue with Light/Dark theme. So the app must always be in Dark mode?

ditek commented 1 year ago

Yes, I already mentioned about that (https://github.com/ditek/MidiSheetMusic-Android/pull/27#issuecomment-1613767156) and you asked about new PR for that (https://github.com/ditek/MidiSheetMusic-Android/pull/27#issuecomment-1615139972). The File Browser tab should request permission for that.

We can change it later to request the permission from within the FileBrowser as you suggested, but in the meantime the way it works now is worse in my opinion. I like the idea of keeping every commit in history as stable as possible, so let's keep the current way until we do it properly. Also, I'm not sure who the back button becoming not usable to exit the app is related to this. I try to put myself in the shoes of the user when testing and those two issues were very frustrating. I might be mistaken about this. See comment bellow.

So the app must always be in Dark mode?

Yes.

ditek commented 1 year ago

I also noticed that you added the whole .idea directory to .gitignore. So there is no consensus in the community regarding this and there is no right or wrong here, but I subscribe to the idea that some files are useful to keep. For example, .idea/codeStyles/Project.xml specifies an indent size of 4, which is useful to keep the code base consistent. Actually, JetBrains suggests to share:

All files under the .idea directory in the project root except the items that store user-specific settings

This is mentioned here along with examples of files to ignore: https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems

So definitely feel free to suggest specific files to ignore :)

ditek commented 1 year ago

Also, I'm not sure who the back button becoming not usable to exit the app is related to this.

I might be mistaken about this. I tested again, and this even happen with Master on API 34. It is not a problem on API 29 with Master or PR. Do you know if they changed something in the latest Android version to cause this?

bmx666 commented 1 year ago

Also, I'm not sure who the back button becoming not usable to exit the app is related to this.

I might be mistaken about this. I tested again, and this even happen with Master on API 34. It is not a problem on API 29 with Master or PR. Do you know if they changed something in the latest Android version to cause this?

The official release from Google is scheduled for the end of August, so we might have to wait until then and exclude API34 until it is available. 👀

bmx666 commented 1 year ago

Now it has dark theme and night mode by default but on API21 and API22 text is displaying with red color.

bmx666 commented 1 year ago

I did rebase of my master branch, squash reformat code and revert back IDEA code-style files, but ignore rest of working and temporary files. Anyway, about Storage permission, I suggest to move this PR into develop branch and fix permission request in File Browser, otherwise without that permission app will crash if user disable storage access in App Info.

bmx666 commented 1 year ago

Now it has dark theme and night mode by default but on API21 and API22 text is displaying with red color.

I found solution, it's a bug in MaterialDrawer, I made PR to fix it -> https://github.com/mikepenz/MaterialDrawer/pull/2797

bmx666 commented 1 year ago

I tested fix with my aar from jetpack.io and it works fine:

dependencies {
    //implementation 'com.mikepenz:materialdrawer:9.0.1'
    implementation files('src/main/libs/materialdrawer-develop-fab6ef6b5c-1.aar')
    implementation "com.mikepenz:fastadapter:5.7.0"
    implementation('com.mikepenz:materialdrawer-iconics:9.0.1') {
        exclude group: 'com.mikepenz', module: 'materialdrawer'
    }
    implementation('com.mikepenz:materialdrawer-nav:9.0.1') {
        exclude group: 'com.mikepenz', module: 'materialdrawer'
    }
}

MaterialDrawer_api21_fix

bmx666 commented 1 year ago

Hi @ditek I rebased my master to include fixed materialdrawer v9.0.2 Now it works fine, no more red color

ditek commented 1 year ago

Wow, that's such a good effort! Sorry again for not being very active. I'll try to take a look again soon and get back to you 🙂

bmx666 commented 1 year ago

There is only one change since the last update - I updated just numbers from 9.0.1 to 9.0.2, I didn't touch a source code at all 😉 The ugly solution with Storage permission request is still presented 😅