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

Add Bulk Exporter #288

Merged
merged 2 commits into from
Oct 28, 2020
Merged

Conversation

psyomn
Copy link
Contributor

@psyomn psyomn commented May 31, 2020

Here's a mock. I'm almost done with a draft of the implementation. I was thinking of having the code replicate the src fs tree into the destination tree.

ie: source looks like this:

src/a/1.ptb
src/b/2.ptb
src/c/3.ptb
src/c/d/4.ptb

into:

dest/a/1.pt2
dest/b/2.pt2
dest/c/3.pt2
dest/c/d/4.pt2

What this implies is if src is dest, the files would be generated alongside in the same directory. Is this sane behavior?

Also here's a mock:
screen

The last box should show messages if some file could not be read, or something weird happens.

Does this course of development seem ok so far? Will implement the actual conversion algo today probably.


TODO (to complete this PR):

  • reconsider directory walking implementation?
  • rename bulk convert
  • add informative error message on stdout + edit box on files which fail to be converted

Sorry, something went wrong.

@psyomn
Copy link
Contributor Author

psyomn commented May 31, 2020

Btw, ignore some of the formatting: I'm going to fix it when things are ready. My editor also removed some whitespace on save; I can revert those changes if you want me to!

Edit: also about filesystem, do you have usage preference? Since we're using C++17, I think there's kind of support for filesystems, but I'm not sure how many compilers do this right now. Should I use boost, or Qt instead?

@cameronwhite
Copy link
Member

I think that seems like a reasonable behaviour! For reference, Guitar Pro has explicit toggles for whether to traverse sub-directories, and whether to generate in the source directory or allow selecting a different folder.

We haven't yet switched to the standard filesystem library, although that's something to look into soon probably. Currently boost filesystem is used for anything in the core libraries which don't depend on Qt, but Qt is fine for anything related to file dialogs etc.

A couple notes: source/app/paths.h has conversion functions for QString -> boost::filesystem, and you could look at PowerTabEditor::setPreviousDirectory() for how to access the directory that file dialogs should be initially set to (this should no longer be cached in that class if it's going to be updated from elsewhere)

@psyomn psyomn force-pushed the feature/add-bulk-exporter branch from 661e391 to 0d5d024 Compare June 6, 2020 23:13
@psyomn
Copy link
Contributor Author

psyomn commented Jun 8, 2020

So I think I'm 90% done. I still need to incorporate some of your feedback, but it's getting there. I'm kind of considering re-writing some of this, by defining some filesystem iterator that could be generic enough to be reused elsewhere here. Unless you think that's overkill.

Anyway I'm going to loop over my code a little more and make sure that I can clean it as much as possible! Thanks for the feedback so far!

@cameronwhite
Copy link
Member

Sounds good, let me know when you'd like me to take another look!

By filesystem iterator, were you referring to the logic in walkAndConvert()? I did notice that boost::filesystem seems to have a recursive_directory_iterator and some utility functions for building relative paths, so perhaps those could be used if they work for your use case?

@psyomn
Copy link
Contributor Author

psyomn commented Jun 23, 2020

Hey there, sorry for the silence -- it's been busy at work. I should have some free time soon to continue and finish this PR!

@cameronwhite
Copy link
Member

No problem!

@psyomn psyomn force-pushed the feature/add-bulk-exporter branch 5 times, most recently from b703ebe to f753ca8 Compare July 19, 2020 23:01
@psyomn psyomn marked this pull request as ready for review July 19, 2020 23:03
@psyomn
Copy link
Contributor Author

psyomn commented Jul 19, 2020

By filesystem iterator, were you referring to the logic in walkAndConvert()? I did notice that boost::filesystem seems to have a recursive_directory_iterator and some utility functions for building relative paths, so perhaps those could be used if they work for your use case?

Yep that's what I was talking about! I'll have to take a closer look at what that exactly does, and will swap to it if it simplifies the code.

I still need to go over some of your other feedback, but in the meantime if you see something that is extremely wrong, let me know!

I'm changing this to ready for review, so I can add the finishing touches, and hopefully we can merge this soon!

Copy link
Member

@cameronwhite cameronwhite left a comment

Choose a reason for hiding this comment

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

Looks good so far! Just made a couple suggestions

source/dialogs/bulkconverterdialog.cpp Show resolved Hide resolved
source/dialogs/bulkconverterdialog.cpp Outdated Show resolved Hide resolved
source/dialogs/bulkconverterdialog.cpp Outdated Show resolved Hide resolved
@psyomn psyomn force-pushed the feature/add-bulk-exporter branch 2 times, most recently from c4bb0f9 to 3a89f97 Compare July 28, 2020 05:41
Comment on lines 97 to 99
// TODO: this should probably handle any supported format.
// check to see if there is a list of file formats
// like this
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my last thing, that I was not able to find a direct answer for. Is there a way (and should I do this), of checking all parsable formats to export to .pt2 (ie, gp5 etc)

Copy link
Member

Choose a reason for hiding this comment

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

I think the closest would be FileFormatManager::findFormat() - you could extend that or provide a similar function that only checks the available importers. FileFormatManager::importFileFilter() is similar but is geared towards building the filter for the file dialog.

@psyomn
Copy link
Contributor Author

psyomn commented Jul 28, 2020

Should be ready for one more pass whenever you have the time!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Closes powertab#212
@psyomn psyomn force-pushed the feature/add-bulk-exporter branch from 3a89f97 to d9276d7 Compare September 25, 2020 18:41
@psyomn
Copy link
Contributor Author

psyomn commented Sep 25, 2020

Hey there I've applied the changes you requested! I think it works properly, will be running some more quick checks but I think this is done!

@psyomn psyomn force-pushed the feature/add-bulk-exporter branch from d9276d7 to cc7db80 Compare September 25, 2020 19:58
@cameronwhite
Copy link
Member

Great! I'll do some testing of it later this weekend

@cameronwhite
Copy link
Member

This looks great!

Just a couple minor things from testing it out:

  • The menu item's label should be Bulk Converter... (with the ellipsis) since it opens a new dialog
  • Maybe there should be some output in the message box if everything completed successfully, just to make it obvious that something happened?
  • The dialog's title just showed up as "Dialog" for me

Screen Shot 2020-09-27 at 6 23 38 PM

@psyomn
Copy link
Contributor Author

psyomn commented Sep 28, 2020

Will do!

The layout looks a little strange. The edit boxes and progress bar seemed cropped. Is this normal?

Thanks for the kind words!

@cameronwhite
Copy link
Member

Yeah, the buttons do look odd - maybe try removing some of the fixed widths / heights in the .ui file?

This change makes it possible to introspectively look at the supported
formats, whatever they may be, and convert the files in question into pt2.
@psyomn psyomn force-pushed the feature/add-bulk-exporter branch from cc7db80 to 4ade724 Compare October 28, 2020 04:26
@cameronwhite
Copy link
Member

The changes look good, thank you! I've still got a couple layout issues on macOS, but it's probably easier for me to just make those changes since I can test them out

@cameronwhite cameronwhite merged commit b152628 into powertab:master Oct 28, 2020
@psyomn
Copy link
Contributor Author

psyomn commented Oct 28, 2020

Hey there, thanks for the merge! I was planning to investigate it a little more today (yesterday I was playing with some of the values like you suggested), but I couldn't figure out what was wrong. Feel free to poke me, and I'll be more than happy to try and help out.

In the mean time I'm going to try out some profiling stuff on memory we talked about in another issue (leak + massif). If there's something you'd prefer I take a look please let me know!

Also, are refactors welcome? For example I think there's a bunch of C++ features we could leverage on, to reduce some of the usage of boost. Would patches like these be welcome?

Thanks for all the support!

@cameronwhite
Copy link
Member

From looking at it a bit more, I think the best fix might be to refactor the layout to use layouts (QHBoxLayout / QVBoxLayout, etc) to arrange the widgets instead of fixed sizes and positions? That should make it more tolerant of the different styles on each platform's widgets

Refactors are definitely welcome! I'm always happy to eliminate some of the boost usage :)

@psyomn
Copy link
Contributor Author

psyomn commented Nov 2, 2020

Thanks! I'll start working on the refactoring this evening!

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

2 participants