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

DolphinQt: Add "File Path" column to Game Grid #8580

Open
wants to merge 4 commits into
base: master
from

Conversation

@AlexApps99
Copy link

AlexApps99 commented Jan 24, 2020

The path is generated by finding a file path that contains the filename, then removing the path to that path.

Example:
File paths: /home/foo/games, /home/foo/Desktop/wii
Game path: /home/foo/games/gcn/ntsc/fun game.gcn
File name listed: gcn/ntsc/fun game.gcn
File name listed: games/gcn/ntsc/fun game.gcn

dir.lastIndexOf uses -2 in the case that the file path ends with a trailing slash.

Fixes https://bugs.dolphin-emu.org/issues/10567

This is my first PR to Dolphin, so a thorough review would be awesome, and let me know if this would be better as its' own column instead of replacing "File name".

@AlexApps99 AlexApps99 requested a review from JosJuice Jan 24, 2020
@AlexApps99 AlexApps99 force-pushed the AlexApps99:master branch from 542e77e to 76ce1cd Jan 24, 2020
@MayImilae

This comment has been minimized.

Copy link
Contributor

MayImilae commented Jan 24, 2020

Definitely make this a new column please.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 24, 2020

Ok, do you have any other feedback?

@MayImilae

This comment has been minimized.

Copy link
Contributor

MayImilae commented Jan 24, 2020

Nothing else for the moment. I'll give it a proper try tomorrow and give you additional feedback then, it's 2am here!

@AlexApps99 AlexApps99 changed the title Add path to File Name column of game grid DolphinQt: Add path to File Name column of game grid Jan 25, 2020
@AlexApps99 AlexApps99 changed the title DolphinQt: Add path to File Name column of game grid DolphinQt: Add "File Path" column to Game Grid Jan 25, 2020
@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Looks good, but is it hidden by default? I imagine this is one of those columns you'd rather default to hidden and allow the user to show it later.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

I think it isn't shown by default, but I haven't verified it

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Please verify then. I'd also appreciate a screenshot as I'm curious how does this display.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Ok, deleted config from ~/.config/dolphin-emu/ and ran it.
It is hidden by default
image

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Screenshots of it in action
image

image

Copy link
Contributor

CookiePLMonster left a comment

As far as I understand, you made it strip "common" parts of the path basing on Game Folders list. If I understand it correctly, then it doesn't work as intended on Windows:

image

image

EDIT:
You probably need to convert those QStrings to paths (QFileInfo?) and work from that.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

The issue is that the path in Settings uses backslashes and the function to get the path uses forwardslashes.

I'll work on a fix right now

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Hence my remark that you probably need to compare those as paths and not strings - it'd most likely save some headache with more edge cases and not just slashes.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Thanks for the advice, will do that

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Is this implementation valid? It removes all symlinks/backslashes so the strings will be correctly formatted.

Otherwise, how would I check if a directory is contained in another? I couldn't find a function for this in QFileInfo's docs

Your feedback is appreciated

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Sorry, I might have been mistaken. Looks like the function you need here is QDir::cleanPath.

So IMO you could erase your last commit by undoing everything in it and just filtering paths with this function, then you should be able to compare everything as you did previously.

As for erasing the commit, you could undo the changes, apply QDir::cleanPath changes, then amend the commit and rename it, then force push (it's fine since it's your own branch).

@AlexApps99 AlexApps99 force-pushed the AlexApps99:master branch from 0fe030d to 36bd8c0 Jan 26, 2020
@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Done

@AlexApps99 AlexApps99 force-pushed the AlexApps99:master branch from 747f4aa to 2726459 Jan 26, 2020
@AlexApps99 AlexApps99 removed the request for review from JosJuice Jan 26, 2020
@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

I gotta go to sleep now (1:30am here), thanks for all of the assistance so far @CookiePLMonster :)

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Unfortunately it still seems to be invalid:

image

I will debug this in a few hours.

Or maybe I am misunderstanding this feature? How exactly do you expect it to work? Basing on your screenshots I expect my paths to be like

Crash Bandicoot...
Mario Kart...
MGS\Metal Gear...
PaperMario...
Copy link
Contributor

CookiePLMonster left a comment

Funny, build artifacts for a force pushed build must have been incorrect since I built your branch locally and it works probably as expected:

image

@mbc07

This comment has been minimized.

Copy link
Contributor

mbc07 commented Jan 26, 2020

Shouldn't it display back slashes instead of forward slashes on Windows?

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

I don't think it matters. I'm not convinced you can get Qt to output a "platform natural" path, and that's what you'd need in this case.

Not true, QDir::toNativeSeparators performs exactly that conversion so we could apply this after comparison/slicing for "better" display style.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

cleanPath will always output forwardslashes, I could either check the platform and replace all forwardslashes with backslashes, or find a different function.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

I think performing comparisons and slicing on "clean" paths is a good idea - but may want to use "native separators" path for solely for displaying it to the user.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Maybe I could put the processed path back into QFileInfo or QDir and get a platform-native relative path

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

No need - QDir::toNativeSeparators should do exactly what you need :)

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Awesome, will add that as soon as I can 👍

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Excellent :)

image

Note to self: Verify that paths display consistently everywhere, as right now in Game Folders I can see paths with both styles. Ideally, they should be stored in "clean" format (so they are cross platform) but stored in "native" format!

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

Hmm now that's interesting, did lint bot spot missing brackets or incorrect indentation? Could be both, depending on what you intended this code to do.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

Yeah, it was missing brackets... Oops

@AlexApps99 AlexApps99 force-pushed the AlexApps99:master branch from cb6343c to ea3c27e Jan 26, 2020
@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 26, 2020

One more nitpick - I see you have edited an example of how it is supposed to work (or it's been there and I am just blind), and it would seem like Game Folder path is not to be included in the shown path at all. Yet, if you see the screens I posted above for me dolphin-games still shows in those paths, despite being part of both game folder paths.

Any idea what's up with that?

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 26, 2020

That is a typo, the path shortening behaviour works as I intended it to
If you'd prefer that I remove the base folder path from the columns, that's fine too

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 27, 2020

@CookiePLMonster should I keep the current functionality or change it to reflect the old example?

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 27, 2020

@CookiePLMonster should I keep the current functionality or change it to reflect the old example?

For me the way it works now is good - let's wait for somebody else (preferably those who can merge) to comment on it too.

@AlexApps99

This comment has been minimized.

Copy link
Author

AlexApps99 commented Jan 30, 2020

What else needs to be done to complete this PR?

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Jan 30, 2020

What else needs to be done to complete this PR?

I would say it's finished. At this point just sit tight and wait until somebody who
a) is involved with UI
b) has merge rights
reviews it.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.