Skip to content

Commit

Permalink
Remove double "/" in SoundManagerConfig::SoundManagerConfig()
Browse files Browse the repository at this point in the history
  • Loading branch information
@daschuer
daschuer committed Jun 13, 2015
1 parent 4ea0cc6 commit 809b933
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/soundmanagerconfig.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ SoundManagerConfig::SoundManagerConfig()
m_deckCount(kDefaultDeckCount),
m_audioBufferSizeIndex(kDefaultAudioBufferSizeIndex),
m_syncBuffers(2) {
m_configFile = QFileInfo(CmdlineArgs::Instance().getSettingsPath() + "/" + SOUNDMANAGERCONFIG_FILENAME);
m_configFile = QFileInfo(CmdlineArgs::Instance().getSettingsPath() + SOUNDMANAGERCONFIG_FILENAME);
}

SoundManagerConfig::~SoundManagerConfig() {
Expand Down

6 comments on commit 809b933

@sblaisot
Copy link
Member

Choose a reason for hiding this comment

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

This was added intentionally.
SettingsPath now rely on QDesktopServices::DataLocation and we can't be sure that is will always end with a / in the future.
double / are usually well handled at OS level and should not be a problem.

It is good practice to always add a / between a path part a filename part.

At least if you revert this one, you should also consider this one:
9c3e304

@rryan
Copy link
Member

@rryan rryan commented on 809b933 Jun 14, 2015

Choose a reason for hiding this comment

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

The cleanest way to do this would be:

QDir(CmdlineArgs::Instance().getSettingsPath()).filePath(SOUNDMANAGERCONFIG_FILENAME)

@daschuer
Copy link
Member Author

Choose a reason for hiding this comment

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

For me it is a question of consistency.

We already assure that getSettingPath returns a tailing slash
https://github.com/mixxxdj/mixxx/blob/1.12/src/util/cmdlineargs.h#L31
https://github.com/mixxxdj/mixxx/blob/1.12/src/util/cmdlineargs.h#L83
https://github.com/mixxxdj/mixxx/blob/master/build/depends.py#L1167

So if we trust our own code the change is correct.

However, returning a pathname with a tailing slash is quite unusual.
So we might want to move to getSettingPath without tailing slash and add it when building
the final path. This adds an extra safety since accidentally additional slash would not hurd because of the smart Qt handling.

The QDir() version handels all odd cases. If we fear to much and think we need them, we have to introduce it everywhere we use getSettingPath().

What is the best solution?

@sblaisot
Copy link
Member

Choose a reason for hiding this comment

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

I agree with you, it's a question of consistency, and this is the reason why I added this "/".
For consistency reasons, we need to have a / between path and filename here, because we have a "/" at the beginning of filename here:
https://github.com/mixxxdj/mixxx/blob/1.12/src/dlgdevelopertools.cpp#L51
https://github.com/mixxxdj/mixxx/blob/1.12/src/analyserwaveform.cpp#L32
https://github.com/mixxxdj/mixxx/blob/1.12/src/controllers/defs_controllers.h#L20
https://github.com/mixxxdj/mixxx/blob/1.12/src/controllers/defs_controllers.h#L25
https://github.com/mixxxdj/mixxx/blob/1.12/src/controllers/defs_controllers.h#L32
https://github.com/mixxxdj/mixxx/blob/1.12/src/main.cpp#L98
https://github.com/mixxxdj/mixxx/blob/1.12/src/main.cpp#L100
https://github.com/mixxxdj/mixxx/blob/1.12/src/mixxx.cpp#L131
https://github.com/mixxxdj/mixxx/blob/1.12/src//library/dao/analysisdao.cpp#L261
etc...

@daschuer
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, we have a bigger problem :-(
All this leads to log entries like this:

Main]: ConfigObject: Could not read "/home/daniel/.mixxx//mixxx.cfg"

Which should be fixed.
For now I prefer to remove the tailing slash from the settingsPath
Do you have fun to take care about this?

@sblaisot
Copy link
Member

Choose a reason for hiding this comment

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

Ok, i'll take care of this, using the cleanest method provided by @rryan
bug filled: lp:1464975

Please sign in to comment.