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

Use Sailfish.Pickers.FolderPickerPage to configure music directories #33

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Use Sailfish.Pickers.FolderPickerPage to configure music directories #33

merged 2 commits into from
Feb 7, 2024

Conversation

smokku
Copy link
Contributor

@smokku smokku commented Dec 17, 2023

Replaces custom AddFolder dialog with Sailfish FolderPickerPage.

image
image

Should help resolve #26 confusion.

@Olf0 Olf0 self-assigned this Feb 3, 2024
@Olf0 Olf0 added the enhancement New feature or request label Feb 3, 2024
@Olf0
Copy link
Contributor

Olf0 commented Feb 3, 2024

@smokku, thank you very much for this and your other contribution (MR #34).

Sorry for the long delay, I was knocked-out by influenza etc. from early December until mid January and now I am trying to catch up with my backlog at work, private stuff and last, but not least SFOS-related stuff.

Note that FlowPlayer's code-base currently supports all releases since 1.1.9 (maybe even 1.1.7), that the GitHub CI workflow compiles for some releases starting with 2.2.0 and that SailfishOS Chum supports all releases since 3.1.0.
Mind that users of a Jolla 1 are stuck at SFOS 3.4.0 and users of devices with community ports of SFOS at various releases.

Thus a mechanism for different code paths to target different SFOS releases has to be employed, before merging this PR.

  • The theoretically more elegant solution is achieving this by ifdefs / ifndefs (or something similar) in the code and / or spec file. This way a single code base covers all SFOS releases. But this scheme can clutter the source code in the long run, if there are many deviations (Jolla tends to often introduce breaking changes). Plus I never implemented anything such or maintained such an implementation.
  • A simpler approach is to handle changes breaking backward compatibility by using different source code branches. Such a scheme is detailed in Storeman-Wiki:Git-commit-workflow. This is a scheme I am using for a couple of projects.
    For an example, see the coding differences between the old (< SFOS 4.2.0: Sailfish.TransferEngine) and new (≥ SFOS 4.2.0: Sailfish.Share) sharing mechanism in Storeman: Storeman@sfos3.3...sfos4.2

I consider implementing the second scheme some day, unless someone (you?) poses a merge request with some (e.g. the first) approach meanwhile. In the latter case, I just have to review, which should be done quicker.

Either way it is necessary to know the exact SailfishOS release, which introduced a change; because this is the minimal release for which switching the code ("… base", when using the second, simpler approach) is feasible. This documentation can be helpful for researching these. Please reference the source(s) of this information by web-links, when providing the result of this research here.
AFAIU, the current, old code base still works with the current SailfishOS release (4.5.0), hence a SFOS release for which switching the code ("… base") is required does not yet exist, correct?

smokku and others added 2 commits February 7, 2024 09:08
Fallback to old code if the Sailfish.Pickers
module is not available in the import path.
@dcaliste
Copy link
Collaborator

dcaliste commented Feb 7, 2024

I rebased on latest devel and try to detect if Pickers are available on the current run. Fallback to the old code if the Pickers are not found. Otherwise, use the new code proposed by @smokku.

@Olf0, if @smokku agrees, you may want to squash the 2 commits for clarity of the history.

@smokku
Copy link
Contributor Author

smokku commented Feb 7, 2024

Fine with me. 👍

@Olf0
Copy link
Contributor

Olf0 commented Feb 7, 2024

you may want to squash the 2 commits for clarity of the history.

BTW, I squash-merge feature-branches by default, unless there is a reason not to, e.g. separate commits provide some kind of clarity to a reader later on.

Here, I would sure squash-merge by all technical aspects, but that obfuscates that there were two different contributors (i.e. a social aspect). Well, I can name you in the commit message, if I do not forget to.

Will review and (likely) merge tonight.

Copy link
Contributor

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

LGTM

@Olf0 Olf0 merged commit 0e29326 into sailfishos-applications:devel Feb 7, 2024
1 check passed
@dcaliste
Copy link
Collaborator

dcaliste commented Feb 8, 2024

Thanks @Olf0 , no problem to loose the ownership of changes on the few lines I modified. That's fair that the commit stay with smokku authorship. He had the idea and proposed the core of the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] No SD card access
3 participants