Pull requests

#6 Declined
Repository
shield007 shield007
Branch
default
Repository
wez wez
Branch
default

New atoms and output as XML option

Author
  1. John-Paul Stanford avatarJohn-Paul Stanford
Reviewers
Description

Added --flavour option for setting the flvr atom. Added --outputXML to output atoms as XML Tweaked the --hdvideo option to take into account 1080p content. Fixed memory corruption bug and stop resulting visual studio warning dialog Added cmake build script Added the ability to set sort atoms via options in the format --sortOrder "name=ArtistName" to avoid a bug with values that start with a '-' symbol.

Comments (10)

  1. Oleg Oshmyan

    I’ve added comments to some specific commits.

    The greater part of this pull request deals with CMake. This isn’t bad per se (I personally don’t like or use CMake much, but I’m sure other people think differently, and in any case it’s up to Wez to decide), but it’s not obvious from the pull request title or the commit messages, so I figured I’d at least point this out here. I think it would have been a good idea to separate CMake changes from the rest so that they could go into a separate pull request and also to mention CMake by name in the relevant commit messages.

    Since I’ve started talking about good commits, I’ll also mention that occasionally your commits contain separate blank lines or trailing white space. Probably not a big deal, especially since everyone commits them once in a while, but avoiding this shouldn’t be hard either.

    For the record, I still intend (heh, is it more than a year now?) to clean up/correct the usage of autotools. I actually have a patch that does most of the job, but there are a few issues remaining and the perfectionist inside me demands that I resolve them before committing.

  2. John-Paul Stanford author

    Thanks for the feed back. Yes their was quite a number of cmake changes I commited. Mostly because cmake simplifies the building of different platforms. I was able to build on this and have a contious integration system that builds AtomicParsley for mac, windows and linux 64/32 bit. I'm hoping other uses will benefit from this as well. I did not remove the auto tools. They still work.

    I should probaly document how to build for the different platforms, though using cmake that's pretty simple.

    Blank space and white lines can be cleaned up if you need me todo that. Guess my editors have different tab setups and the blank space on end of the lines should be easy to strip.

    1. Oleg Oshmyan

      One more thing I’ve just noticed: as you can see in the commit list below, the first line of a commit message by convention describes the whole commit. Your first commit, for example, adds --flavour and --outputXML, but this isn’t clear from the first line of its commit message, which makes it hard to determine which commit adds XML output by looking at the commit list.

  3. Wez Furlong repo owner

    There's a lot going on in this pull request, so I'm going to reject it on the grounds that it should get broken up into smaller pieces, each of which should be able to be assessed on their own. Please don't take that badly; I value your contributions!

    Regarding cmake, I personally see very little value in it, don't use it and am unlikely to be motivated to maintain a working cmake build. If you'll commit to maintaining it, I'll accept the cmake specific changes. If they end up falling into an unmaintained state, I'll remove them.

    Regarding coding style (which is hard to call out, given the nature of the code we've inherited!) I have a couple of comments:

    uint16_from_string_base(item_stat,0,10);

    Can we make the 0, 10 parameters go away? They don't seem to be used anywhere in the commits below and just make things seem noisy; how about uint16_from_string(const char *ptr) instead?

    The files you added should have "better" copyright/license headers; one has just "jp" as an attribution and is missing the license, the other is copied from metalist.cpp and credits puck_lock instead of yourself.

    Please make the --hdvideo also accept true/false so that folks that have integrated against AP don't get broken when they upgrade? It seems like false should mean SD and true => 720p

    Thanks!

    1. John-Paul Stanford author

      Ok,fair enough. I'll probably revert all my changes and commit them again in a cleaner fashion. The cmake support is basically one file. So I'm happy to maintain that and if you do decide it's something you don't want that's OK, I can always keep that in my local copy. I've found it to be a big help when building for multiple platforms, but thats my opinion.

      When I recommit the changes I'll address the issues you've both mentioned in the code style.

      Cheers!

  4. John-Paul Stanford author

    I've now reverted and recommited all my changes. So I've updated the pull request. Hopefully it's a lot clearer now as each commit is a isolated bug fix or feature.

    I make the changes you suggested to --hdvideo to be compatible with previous versions.

    I fixed broken copyright headers.

    Also changed the functions in conversions.h as you suggested.

    Cheers, JP.

    1. Wez Furlong repo owner

      You'll probably hate me for asking this, but can you make a separate pull request for each of the different bits? bitbucket isn't showing me the diffs here in the UI now either (presumably too big?)

      1. John-Paul Stanford author

        This is the first time I've really used bitbucket site, so I've not sure how to make pull requests for each of my commits. Do I need to delete my fork and recommit stuff again, but every time I make a commit also make a pull request?

        I could send you the changes as patches is that's easier?

        I probably have other changes to make as I've found a few other bugs in the command line parameter parsing I'd like to fix.

        1. Wez Furlong repo owner

          Sorry, I dropped the ball on this; I've had a lot of "real life" going on :-/

          Regarding breaking it apart, if you're still interested in doing the work, I think the easiest way to get that in is to have one branch with the cmake work, another one for the hdvideo option and so on, with each one based from the main AP repo sources--in other words, they all branch from the same code, but each branch only has the changes for the particular feature you want to get in.

          Make sense?

          1. John-Paul Stanford author

            At the moment I'm kind of busy also so I'm happy maintaining my brach. I've fixed a couple of issues over the last few months.

            It will be quite a bit of work to split all my changes into different barnches so I'll have to wait till I have some free time. Hopefully I can revist this before too long.

Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.