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

Better default working directory #4288

Merged
merged 5 commits into from Apr 20, 2018

Conversation

Projects
None yet
5 participants
@tresf
Member

tresf commented Apr 8, 2018

Better behavior of default working directory:

  • Fresh installs of LMMS (e.g. no ~/.lmmsrc.xml) will default to the more browsable, more sane and less conflicting ~/Documents/lmms.
    * No longer prompt to create the home directory.

Note: The Qt4 logic is currently untested.

Closes #1135

@PhysSong

This comment has been minimized.

Member

PhysSong commented Apr 8, 2018

Fresh installs of LMMS (e.g. no ~/.lmmsrc.xml) will default to the more browsable, more sane and less conflicting ~/Documents/lmms.

👍

No longer prompt to create the home directory.

Could you explain why did you change that?

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 8, 2018

Note: The Qt4 logic is currently untested.

Tested, works fine.

@tresf

This comment has been minimized.

Member

tresf commented Apr 8, 2018

No longer prompt to create the home directory.

Because asking newcomers a confusing question on first install is a bad first impression.

@PhysSong

This comment has been minimized.

Member

PhysSong commented Apr 9, 2018

No longer prompt to create the home directory.

Because asking newcomers a confusing question on first install is a bad first impression.

I agree, but I found some situations where that behavior is undesirable. When the working directory is set by user and moved/deleted/temporarily unable, prompt is still valid.
I suggest checking ConfigManager::inst()->value( "app", "configured" ).toInt() if you want to detect fresh install.

@tresf

This comment has been minimized.

Member

tresf commented Apr 9, 2018

When the working directory is set by user and moved

A warning is valid. A blocking dialog is the wrong way.

/deleted/

I'm not sure how this is helpful to anyone in this case. They can delete the NEW "bread crumbs" if it's undesired. It's of zero consequence or near-zero consequence.

temporarily un[avail]able

This dialog offers no contextual evidence that it's temporarily unavailable and the mkdir will silently fail. I'm not sure the use-case here.

I suggest checking ConfigManager::inst()->value( "app", "configured" ).toInt() if you want to detect fresh install.

That would create unpredictable behavior. It's better to just create the directory in 99.9% of use-cases. We shouldn't be coding to the 1%.

I assume the atypical use-case you're describing is a user-space mountpoint where a drive is mounted in ~ instead of /media/, etc? (I'd call this atypical, and a warning should sufice, but you're this dialog wasn't intended to serve this purpose). Apps should create sane, working directories. Arguably, the .lmmsrc.xml should live directly inside the working directory too, it has no place in ~/.lmmsrc.xml, but I plan to address that as part of DataFile moving from XML to JSON, related: #3981.

@PhysSong

This comment has been minimized.

Member

PhysSong commented Apr 9, 2018

I use a NTFS partition as the working directory. If LMMS creates the working directory without prompt when the partition is not mounted, it will create a directory which was the path for the partition. That can break several things(bookmarks, other programs' data directories).

I think it's unusual, but I think it'd be better to cover such cases.

@musikBear

This comment has been minimized.

musikBear commented Apr 9, 2018

Arguably, the .lmmsrc.xml should live directly inside the working directory too, it has no place in ~/.lmmsrc.xml

Imo in Data, its wrong to force-install things in C: when a user has chosen a different partition as install-point.

@tresf

This comment has been minimized.

Member

tresf commented Apr 9, 2018

Imo in Data

I'm not sure what you're talking about. There's no such thing as data. Perhaps you're referring to %APPDATA%, but that's still explicit.

its wrong to force-install things in C:

Nothing's being force-installed anywhere, please don't make blanket statements.

@tresf

This comment has been minimized.

Member

tresf commented Apr 9, 2018

I use a NTFS partition as the working directory. If LMMS creates the working directory without prompt when the partition is not mounted, it will create a directory which was the path for the partition. That can break several things(bookmarks, other programs' data directories).

Normally user-space doesn't have access to write to mount points unless fstab has mounted them e.g.

I assume the atypical use-case you're describing is a user-space mountpoint where a drive is mounted in ~ instead of /media/, etc?

Unless there's some fuse stuff going on. If you can explain how LMMS has access to write to an unmounted directory, I'd like to know.

I think it's unusual, but I think it'd be better to cover such cases.

How though? That's my problem is that you're exploiting a warning to tell you that a drive isn't mounted and that's never what it was intended for.

If you want a warning because a custom location that you HAVE ALREADY SET is missing, that seems like a different feature. We shouldn't be displaying these dialogs before the software has loaded. It's bad enough we popup that ridiculous settings dialog. The software should just load to the daw first time, every time. Any other end-user experience is 1990s. :)

tresf added some commits Apr 10, 2018

@tresf

This comment has been minimized.

Member

tresf commented Apr 10, 2018

Tested this for a day, accidentally clicked the wrong folder and created a bunch of breadcrumbs. I don't like the dialog and the experience should be better but I'll have to agree with @PhysSong and leave it out of scope for now.

Reverted and ready for review/merge.

@musikBear

This comment has been minimized.

musikBear commented Apr 10, 2018

There's no such thing as data

Now you confuse me:
:..LMMS \data
Thats how my installation looks
Is the folder 'data' not default?
-/-
in respect to the installed files on C i refer to
cfiles
Those are installed here without me deciding. Perhaps it wrong to call it 'force-installed', but it is not me who made the descission to place files here.
@tresf

@tresf

This comment has been minimized.

Member

tresf commented Apr 10, 2018

@musikBear, that's unrelated to the working directory. Install LMMS wherever you want. :)

@tresf

This comment has been minimized.

Member

tresf commented Apr 10, 2018

:..LMMS \data

The problem @musikBear is that you are making up terms. Yes the skeleton directory is created automatically. That's done as a convenience so that when you save something such as a preset or a sample you don't have to create the directory -- or worse -- you create it in a non-relative path and samples and plugins won't load.

who made the descission to place files here.
@tresf

The dev team did. Otherwise the folder structure is unobvious and relative paths potentially break. This is a stop-gap courtesy until we introduce portable project files.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 10, 2018

Shouldn't we probe for ~/lmms or ~/Documents/lmms when starting with no .lmmsrc.xml ? You'd probably want to default to an existing working directory.

@tresf

This comment has been minimized.

Member

tresf commented Apr 10, 2018

Shouldn't we probe for ~/lmms or ~/Documents/lmms when starting with no .lmmsrc.xml ? You'd probably want to default to an existing working directory.

Sure. ~/Documents/lmms doesn't need a probe because that's the new default. #1135 is what inspired this change so we'd need to know what to match on so we don't accidentally select the code repository.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 10, 2018

Look forQDir::home().absolutePath() + "/lmms/projects/" ?

@tresf

This comment has been minimized.

Member

tresf commented Apr 11, 2018

Look forQDir::home().absolutePath() + "/lmms/projects/" ?

Done via 052eedc. Ready for testing/review.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 11, 2018

Works as intended.

@tresf tresf closed this Apr 20, 2018

@tresf tresf deleted the tresf:workingdir branch Apr 20, 2018

@lukas-w

This comment has been minimized.

Member

lukas-w commented Apr 20, 2018

@tresf Why did you close this?

@tresf

This comment has been minimized.

Member

tresf commented Apr 20, 2018

@tresf Why did you close this?

Branch cleanup, it was a mistake.

@tresf tresf restored the tresf:workingdir branch Apr 20, 2018

@tresf tresf reopened this Apr 20, 2018

@tresf tresf merged commit 18a4346 into LMMS:stable-1.2 Apr 20, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@tresf tresf deleted the tresf:workingdir branch Apr 20, 2018

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