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

enabled drag and drop support for passwords and passwordfolders #245

Merged
merged 29 commits into from Dec 14, 2016

Conversation

Projects
None yet
3 participants
@YoshiMan

YoshiMan commented Dec 1, 2016

see issue #239
we are now able to move files and directories across the passwordstorage.
e.g.
move folder1 into folder2
move file1 into folder1
move file1 onto file2 -> opens dialog with overwrite question
move folder1 onto file1 -> not allowed/ doesnt make sense

Janosch Knack added some commits Dec 1, 2016

Janosch Knack
Janosch Knack
Janosch Knack
@annejan

This comment has been minimized.

Member

annejan commented Dec 2, 2016

Selecting No in an overwrite dialog removes the file you were attempting to move . .
Does feel nice (when not hitting such ^^ cases) . .

I was using pass (not "native") if that matters . .

@YoshiMan

This comment has been minimized.

YoshiMan commented Dec 3, 2016

hm. i cant reproduce. we have 3 different types. Usage with pass settings (pass mv) , usage with git seetings (git mv), and none of them (filesystem move). maybe you could attach your settings, with sensitve data stripped out.

@YoshiMan

This comment has been minimized.

YoshiMan commented Dec 3, 2016

This pull request shouldnt be merged.
We have some wired behaviour the output of pass mv command is piped into the password file.... -.-
And i could reproduce your error.
I'll have to work on it.

@YoshiMan

This comment has been minimized.

YoshiMan commented Dec 3, 2016

oooh. I know the problem @annejan described...
its a "pass" problem.
If we start the pass-script from QtPass, the pass-scripts detects, that there is no stdin ([! -t 0], see http://www.tldp.org/LDP/abs/html/fto.html) and uses the force mode (-f) instead of interactive (-i).

pass-script-excerpt

local interactive="-i"
[[ ! -t 0 || $force -eq 1 ]] && interactive="-f"

I'll have a look at a workaround tomorrow.

Janosch Knack added some commits Dec 4, 2016

Janosch Knack
Merge branch 'master'
Conflicts:
	localization/localization_ar_MA.ts
	localization/localization_cs_CZ.ts
	localization/localization_de_DE.ts
	localization/localization_de_LU.ts
	localization/localization_el_GR.ts
	localization/localization_en_GB.ts
	localization/localization_en_US.ts
	localization/localization_es_ES.ts
	localization/localization_fr_BE.ts
	localization/localization_fr_FR.ts
	localization/localization_fr_LU.ts
	localization/localization_gl_ES.ts
	localization/localization_he_IL.ts
	localization/localization_hu_HU.ts
	localization/localization_it_IT.ts
	localization/localization_lb_LU.ts
	localization/localization_nl_BE.ts
	localization/localization_nl_NL.ts
	localization/localization_pl_PL.ts
	localization/localization_ru_RU.ts
	localization/localization_sv_SE.ts
	localization/localization_zh_CN.ts
Janosch Knack
@annejan

This comment has been minimized.

Member

annejan commented Dec 6, 2016

Wow, thank you for your hard work @YoshiMan
This feels really nice using it . . Going to be "playing" with it for another couple of hours.

@YoshiMan

This comment has been minimized.

YoshiMan commented Dec 6, 2016

currently its not merged correctly, because there were some merge conflicts. ill do this week, so the real testing can be done at the weekend. (hopefully the master branch stays stable ;) )

@annejan

This comment has been minimized.

Member

annejan commented Dec 6, 2016

There will be some changes to master, at-least to try and fix #243 and #248

Janosch Knack added some commits Dec 8, 2016

Janosch Knack
Merge branch 'master'
Conflicts:
	imitatepass.cpp
	localization/localization_ar_MA.ts
	localization/localization_cs_CZ.ts
	localization/localization_de_DE.ts
	localization/localization_de_LU.ts
	localization/localization_el_GR.ts
	localization/localization_en_GB.ts
	localization/localization_en_US.ts
	localization/localization_es_ES.ts
	localization/localization_fr_BE.ts
	localization/localization_fr_FR.ts
	localization/localization_fr_LU.ts
	localization/localization_gl_ES.ts
	localization/localization_he_IL.ts
	localization/localization_hu_HU.ts
	localization/localization_it_IT.ts
	localization/localization_lb_LU.ts
	localization/localization_nl_BE.ts
	localization/localization_nl_NL.ts
	localization/localization_pl_PL.ts
	localization/localization_ru_RU.ts
	localization/localization_sv_SE.ts
	localization/localization_zh_CN.ts
	pass.cpp
	qtpasssettings.h
	realpass.cpp
Janosch Knack
Janosch Knack
Janosch Knack
Janosch Knack
Merge branch 'master'
Conflicts:
	imitatepass.cpp
	localization/localization_ar_MA.ts
	localization/localization_cs_CZ.ts
	localization/localization_de_DE.ts
	localization/localization_de_LU.ts
	localization/localization_el_GR.ts
	localization/localization_en_GB.ts
	localization/localization_en_US.ts
	localization/localization_es_ES.ts
	localization/localization_fr_BE.ts
	localization/localization_fr_FR.ts
	localization/localization_fr_LU.ts
	localization/localization_gl_ES.ts
	localization/localization_he_IL.ts
	localization/localization_hu_HU.ts
	localization/localization_it_IT.ts
	localization/localization_lb_LU.ts
	localization/localization_nl_BE.ts
	localization/localization_nl_NL.ts
	localization/localization_pl_PL.ts
	localization/localization_ru_RU.ts
	localization/localization_sv_SE.ts
	localization/localization_zh_CN.ts
	pass.h
	realpass.cpp
Janosch Knack
Janosch Knack
fixed insrt of password
avoid empty args.
@annejan

This comment has been minimized.

Member

annejan commented Dec 8, 2016

Wow . . great work on this . .
Will have some time to be able to "play" with it this evening . .

Only other thing on my slate is currently updating the FreeBSD port ;)

Janosch Knack added some commits Dec 14, 2016

Janosch Knack
Merge branch 'master'
Conflicts:
	localization/localization_ar_MA.ts
	localization/localization_cs_CZ.ts
	localization/localization_de_DE.ts
	localization/localization_de_LU.ts
	localization/localization_el_GR.ts
	localization/localization_en_GB.ts
	localization/localization_en_US.ts
	localization/localization_es_ES.ts
	localization/localization_fr_BE.ts
	localization/localization_fr_FR.ts
	localization/localization_fr_LU.ts
	localization/localization_gl_ES.ts
	localization/localization_he_IL.ts
	localization/localization_hu_HU.ts
	localization/localization_it_IT.ts
	localization/localization_lb_LU.ts
	localization/localization_nl_BE.ts
	localization/localization_nl_NL.ts
	localization/localization_pl_PL.ts
	localization/localization_ru_RU.ts
	localization/localization_sv_SE.ts
	localization/localization_zh_CN.ts
	mainwindow.cpp
	realpass.h
Janosch Knack
bug fix
lupdate qtpass.pro
@annejan

This comment has been minimized.

Member

annejan commented Dec 14, 2016

Question @YoshiMan (without going too deeply into the code) are files (optionally) re-encrypted if their new folder's .gpg-id options demand it?
And folders?

For me currently most user management things seem broken, might be my environment, something to do with gpg or something else, needs more investigation.
But I'm tempted to merge this work (and after that my pull request) if you want me to 😬

@annejan annejan merged commit 11537c2 into IJHack:master Dec 14, 2016

3 of 4 checks passed

Snap CI (app) The Snap CI build failed on Dec 14, 2016!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linthub Linthub has cleared the pull request, no code style suggestions found.
Details
@YoshiMan

This comment has been minimized.

YoshiMan commented Dec 14, 2016

@annejan. damn... they are not, if you use git settings. And with neither pass nor git, they arent reencrypted...
So it needs work again... :(
hope i can fix it tomorrow.

@annejan

This comment has been minimized.

Member

annejan commented Dec 14, 2016

Don't worry about it :) And massive thanks on the work so-far.
I have already merged and will merge the unit testing branch as-well . .

Please open another PR when you have the re-encryption working . .

@YoshiMan YoshiMan deleted the YoshiMan:feature/moving_items_(reordering_folders) branch Dec 15, 2016

@tezeb

This comment has been minimized.

Contributor

tezeb commented on mainwindow.cpp in 0d61c56 Jan 3, 2017

Sorry for late review, but this does not seem right.
It won't work when user changes backend(ie. pass->native tools or other way around).
Qt connections are made using object, so here we connect events to whatever is configured at startup, but it won't 'reconfigure' automatically when backend changes(btw. there was a comment about doing it properly, which was also removed).

@tezeb

This comment has been minimized.

Contributor

tezeb commented on qtpasssettings.cpp in 0d61c56 Jan 3, 2017

Why this method instead of constructor? It's against RAII.
It also intruduces bug as it's not set for both realPass and imitatePass, because it's called only once.

This comment has been minimized.

Contributor

tezeb replied Jan 3, 2017

I checked master and it looks better there, as connects are done inside constructor, but the bug is still there, as it's not called for both backends.

@tezeb

This comment has been minimized.

Contributor

tezeb commented on pass.cpp in 7a90685 Jan 3, 2017

I don't think that's the correct place for this method(same for next two). It breaks interface, as they might be called from inside subclasses(ie. we can call it from ImitatePass, so what kind of imitation that would be?).

This comment has been minimized.

YoshiMan replied Jan 12, 2017

yeah. You are right, will be fixed with #253.
I like your code review. good job

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