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

Latest Download object is now unique #308

Merged
merged 2 commits into from May 11, 2011
Merged

Latest Download object is now unique #308

merged 2 commits into from May 11, 2011

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented May 10, 2011

It avoids the automatic smarts of objectWithString and sets everything up manually so the file it references won’t be affected.

…t with the actual file it refers to - partially fixes 306
@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 10, 2011

Is there a reason why you didn't use fileObjectWithPath? That might overwrite the existing file object, but that wouldn't matter, since it would overwrite it with the same thing.
Or you could even try to get the object from the catalog if it's in there?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 10, 2011

I wouldn't suggest looking in the catalog. There's no guarantee the downloads folder will be in somebody's catalog, and I wouldn't say it's worth checking.

I think fileObjectWithPath sounds like a good idea, from what I've seen with the QSMountedVolumesProxy (it actually uses fileObjectWithArray, but that's because there could be more than one mounted volume)

@skurfer
Copy link
Member Author

@skurfer skurfer commented May 10, 2011

I didn’t use fileObjectWithPath because I didn’t know about it. :) I knew objectWithName would do what I asked and nothing more, so that seemed safest.

Looking at fileObjectWithPath, it seems pretty smart. It first checks the catalog with [QSObject objectWithIdentifier:path] and returns that if found. However, it creates a new object using initWithArray which warns “this function could create dups”. I suppose we don’t need to worry about that in this case since it will only run on a path that wasn’t in the catalog.

The current method seems to be working. Do you want me to change it? I have no strong feelings either way.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 10, 2011

I don't have strong feelings about this either. But fileObjectWithPath sounds like the right tool for the job, so yes, I'd like you to change it :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 10, 2011

No strong feelings here, but I agree :)
If QS has tool for dealing with file objects, why not use them? It means
that if we ever change the way QS interacts with file objects we'd only ever
need to change 'fileObjectWithPath:'

On 11 May 2011 04:46, HenningJ <
reply@reply.github.com>wrote:

I don't have strong feelings about this either. But fileObjectWithPath
sounds like the right tool for the job, so yes, I'd like you to change it
:-)

Reply to this email directly or view it on GitHub:
#308 (comment)

@skurfer
Copy link
Member Author

@skurfer skurfer commented May 11, 2011

Here you go. It seems to work well.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 11, 2011

I see no problems with this. Looks good. Merged.

pjrobertson added a commit that referenced this issue May 11, 2011
Latest Download object is now unique
@pjrobertson pjrobertson merged commit 74436de into quicksilver:master May 11, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants