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

Allows to change the db with an already existing db #252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LoicCadour
Copy link
Contributor

While switching between medInria, MUSIC and MUSIC Viewer on Windows (I don't think I had this behavior on Mac), I had my db location changed every time (db directory = ../organisationName/applicationName). Since my MUSIC Viewer db was new and empty, I wanted to select an already existing db (e.g medInria). But it is impossible because, from the source code, a valid db directory is an empty one.
In this PR, I try to fix that by first checking whether the selected directory contains a db file.
I also removed the "previous db deletion" because one may want to switch between db without deleting it.
Finally, I added a label warning that the changes will be effective after restart of medInria.

@@ -587,13 +592,6 @@ bool medDatabaseController::moveDatabase( QString newLocation)
qDebug() << "Restarting connection...";
this->createConnection();
}

// now delete the old archive
if(medStorage::removeDir(oldLocation))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say you may still want to delete the old database if the folder you moved to was empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean if you create a new db, you want the old db to be deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that would sound logical to me : if you're creating a new folder, then you wanted to move the database. If not, that means you want to reuse an older one and therefore keep the old one as well. But I'm open to discussion on this. In terms of code, it's a very easy change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I see your point.
But can we have old db if we cannot create a new one without deleting an old one?
(In my case, I got old db because they were created when compiling medInria, MUSIC, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't merge until resolution on this topic.

For me - deleting the old DB should be a prompt, rather than mandated. This permits us to preserve DBs if we so choose. Users won't change DBs frequently enough to be bothered by an extra step?

If this is acceptable, let me know and I'll implement the change myself.

@fvichot
Copy link
Contributor

fvichot commented Sep 15, 2015

Hola chicos, just passing through with my 2 cents (well 2 centavos, I'm in Bolivia): the whole business of moving DB when a new directory is selected seems a little silly. When the DB path is changed, medInria should either just use whatever is there as the new DB, or create a new empty one if nothing was there, but not move the whole DB to the new folder.

If the user wants to move the DB to a new place, he can just use his file manager from his OS, and then update the path when opening medInria the next time. Moving files around is so prone to disaster, medInria should really stay away from that. In your case, @LoicCadour, you would just start MUSIC and MUSIC Viewer, update their DB path to the medInria one, and from then on, all updates to this DB would be shared by all. Solved. Obviously, don't open more than one of these software at once. But that's either managed by @papadop use of single-instance app he implemented a while ago, or if that mecanism is not shared across Music and medInria, maybe add a lock to the DB so that only one app can open it at the time.

...Yes, I'm still watching everything. 👀

Well, not really, I just got curious if you lot had managed a new release in my absence, and I ended up reading some pull requests. Bad habits die hard I guess ;)

Hasta luego chicos!

mathildemerle added a commit to mathildemerle/medInria-public that referenced this pull request Jun 28, 2017
[DB] serie->series, and rm empty line in DB combobox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants