Resectioning sequences #35

bookdude13 · 2016-08-18T23:27:37Z

Implemented resection-sequence command. I'm planning on doing this pull request myself, unless @rjayatilleka has time. Addresses #31

rjayatilleka · 2016-08-19T04:00:51Z

src/project_types/permissions.rs

+                    let project = try!(utils::read_protonfile(None::<&Path>));
+                    match project.find_sequence_by_name(&seq_name) {
+                        Some(seq) => {
+                            let in_range = section_num > 0 && section_num <= seq.num_sections;


Your range is (0, num_sections]. The norm is [0, num_sections). Did you make it count from 1 intentionally? I think its best to have the cli use a numbering from 0. The GUI can display a numbering from 1 to its users if necessary.

Yes, that was intentional. I think the original comment in the README said to start at 1, so I did. Makes more sense to start at 0 though. This applies to the actual section files as well, right? So sequenceA/sequenceA_section0?

rjayatilleka · 2016-08-19T04:17:29Z

Hmm. I wonder if we should change the API a bit.

set-permission <admin-key> (add | remove) <name> <permission> [<target>]

becomes

set-permission <admin-key> (add | remove) <name> Administrate
set-permission <admin-key> (add | remove) <name> EditSeq <target-seq>
set-permission <admin-key> (add | remove) <name> EditSeqSec <target-seq> <target-section>

Then you just have to construct a Permission like so:

pub enum PermissionEnum 
     Administrate,
     EditSeq(String),
     EditSeqSec(String, i32),
 }

This makes validate_permission simpler as you can take out all of the parsing, type-checking, and conversion code, which is what we wanted docopt for anyway. validate_permission just has to validate against the Protonfile. I definitely prefer this as it follows static typings ethos of 'Make bad states unrepresentable` as well.

rjayatilleka · 2016-08-19T04:18:42Z

And don't count me out just yet. I can definitely keep on code reviews at least.

rjayatilleka · 2016-08-19T04:46:18Z

src/project_types/project.rs

+                }
+                return Ok(new_project);
+            }
+        }


match new_project.sequences.iter_mut().find(|seq| seq.name == name) { None => Err(Error::SequenceNotFound(name.to_owned())), Some(ref mut seq) => { seq.resection(num_sections); Ok(new_project) } }

Possibly a double borrow issue on the Ok line, so you can move that out if you need to.

Soo much cleaner. Didn't know about iter.find()

rjayatilleka · 2016-08-19T04:49:12Z

Will continue tomorrow.

rjayatilleka · 2016-08-19T04:56:43Z

src/project_types/sequence_section.rs

    pub num_frames: u32,
    pub editor: Option<User>,
    pub data: Vec<Vec<u8>>, // Row is channel, column is frame    
 }

 impl SequenceSection {
    /// Write the sequence section to a file
-    pub fn write_to_file(&self, seq_directory_name: &str) -> Result<(), Error> {
+    pub fn write_to_file(&self) -> Result<(), Error> {
        let pretty_json = json::as_pretty_json(&self);


What does a Vec<Vec<u8>> even look like in json?

The Vec turns into a JSON array, so an array of arrays

Oh, k. I'm going to make an issue, that should change to some binary format.

rjayatilleka · 2016-08-19T05:02:42Z

K, I got through everything besides the actual resectioning code. I think you should respond to some the of the comments I made above first though, particularly the caching editors one and the API/Permission one.

bookdude13 · 2016-08-19T13:14:32Z

I really like the new API/Permissions idea. It makes things a lot simpler, and it removes a lot of the code I was kind of questionable about (e.g. the giant verify function in Permission). As mentioned in comments, I agree that we should remove the editor cache for now, since it adds unnecessary complexity and prematurely optimizes. I will work on these changes today/tonight, and hopefully will have a response by tomorrow.

bookdude13 · 2016-08-19T13:15:53Z

And I won't keep you out again until you say you can't keep up 😄

…tests failing

bookdude13 · 2016-08-19T19:05:26Z

Ready for review

rjayatilleka · 2016-08-20T08:26:56Z

README.md

@@ -11,11 +11,8 @@ Command line interface to manipulate ProtonLights projects.
 - `id-user <private-key>`: Identify user by ssh key (public key in repo)
 - `list-permissions <private-key>`: Get list of user's permissions
 - `set-permission <admin-key> (add | remove) <name> <permission> [<target>]`: Change user permissions


Update readme.

rjayatilleka · 2016-08-20T09:05:37Z

I still need to go through the actual resection code. Again. Tomorrow. I think my brain melted while watching Jason Bourne. That movie brought up so many instances of Hollywood Hacking that I couldn't stop myself from constantly thinking, 'NO, the internet doesn't work like that.'

bookdude13 · 2016-08-20T14:31:36Z

Alright, sounds good. I will also review my re-sectioning code. Did the other stuff.

rjayatilleka · 2016-08-25T02:29:33Z

I'm just gonna give you a LGTM. The tests pass, and I doubt there's anything major that needs to change in the resection code.

bookdude13 · 2016-08-25T14:10:19Z

I talked to Michael Rosplock, and he suggested we allow sequences to be sectioned by either channels or time, depending on the song. Should I add this, open a new issue, consider this a version 2 feature, or not do this? I think it makes sense. Would have to tweak the command to have (channel | time) as an argument. Thoughts?

bookdude13 · 2016-09-19T14:49:38Z

Discussed elsewhere. Ended up deciding for 2d vector of sequence section chunks.

* Added name to authors (#4) * Name (#7) * Added Vlad to authors list (#10) * Overhaul of structure to use DAOs and database for backend storage (#11) * Initial database implementations (#12) * Updated README (#14) * Working code for 2016 show (#19) * Created initial 'it works' test * Restructured into separate tests. 'is-working' tests complete? * Restructured 'all working' test * Added more tests. Reorganized again * One more test * Added test for seq sec editor added on perm change. Implemented; passing * All tests implemented except working and 0 sections * All tests passing * Restructured so sequence not returned on resection * Converted main code to new permissions api * Tests using new api. Removed check for sequence section editor. Some tests failing * All tests passing * Removed editor from sequence_section type * Tests now want seq sec in range [0, numSections) * SeqSec now 0-indexed. Removed last traces of editor field * Updated README and fixed error in main * Added checks * Moving towards Fixtures instead of 2d blocks of sequence data * Restructured to use DAOs and database setup instead of storing in flat files * Added initial database backup * Postgres connection working in channel dao * Generalized common functionality * Updated names, public instead of private key, uid for list-permissions * Init project returns generated public key * Updated database definition * Looser Cargo package versions for upgrading/good documentation * Adding initial user mostly implemented. Util function to gen pub/priv keys * Store both pub/priv key. Trim before compare. Get uid from key started * Permission handling stuff. PermissionEnum is string now for simplicity * new-user fully implemented * A little cleanup of tests; moving towards using again * Removed unused imports. New return type for get-uid * PermissionEnum updated. Preliminary work on add_sequence done * new_sequence working? * get_sequence command added to main * new-layout function set up * Reading layout from file. Locations and rotations checked * Adding layout implemented. Transactions/removing not implemented * Layout as of 11/21/2016 added * Project in db behind DAO now as well. General cleanup * set-sequence-layout working * Existential dao functions * Channel data now in separate table * Moved db backups to separate folder * get-layout-id command added, not implemented * Finished implementing add-sequence * get-playlist-data implemented * new-vixen-sequence working. Nullable values changed to options. * Updated Cargo.toml * Added seqid to playlist output * Changed the format of playlist data * Added music file to playlist outputted data * Made functional again after rebase * Added sequence duration to Sequence object for accurate num_frames calc * Added internal channel to Channel type and layout reading * Patching stuff * Added current db backup with internal channel * Patching working now; hackish fix * Added patch file * Updated layout and patch * Working show backed up * Stopped copying music over * Using postgres FUNCTION for patching * Added DB with FUNCTION in last commit added * New patch * Added newer patches * main.rs alphabetized. Added get-project command * Cleaned up warnings * More verbose patching * Added SQL to reset database to clean state * Fixed error with reading file as string * WORKINGgit st! * Working show backup * Adding admin key for 1202 backup * Actual working show backup (whoops) * add-sequence turned into insert-sequence * Udated patch * Print statemets for verbosity * 12_03 patch * Different format for outputting playlist data * Removed public keys from repo * New sequences don't copy their music files to a Music/ directory * Version now 0.19.1 as per SemVer (#33) * Adding beginning of testing framework post-overhaul (#32) * Documentation (#28) * Removed rebase artifacts. Updated version in Cargo.lock (#34) * Permissions checks moved from the second-tier functions to main.rs (#35) * Permissions checks moved from the second-teir functions to main.rs * Fixes issues to moving permissions check * Updated version numbers for dependencies (removing wildcards) (#36)

bookdude13 added 11 commits August 12, 2016 22:38

Created initial 'it works' test

441ecbc

Restructured into separate tests. 'is-working' tests complete?

f97327b

Restructured 'all working' test

d62314b

Added more tests. Reorganized again

33f9ef8

One more test

9a29661

Added test for seq sec editor added on perm change. Implemented; passing

6d34109

All tests implemented except working and 0 sections

6568a8b

Resectioning to 0 sections not allowed

cf3ef58

All tests passing

ed38573

Restructured so sequence not returned on resection

c93b3c5

Updated README

26485bc

bookdude13 self-assigned this Aug 18, 2016

Added debug message

a23a9cf

rjayatilleka reviewed Aug 19, 2016
View reviewed changes

rjayatilleka assigned rjayatilleka and unassigned bookdude13 Aug 19, 2016

rjayatilleka reviewed Aug 19, 2016
View reviewed changes

bookdude13 added 8 commits August 19, 2016 08:50

Converted main code to new permissions api

fbb946b

Tests using new api. Removed check for sequence section editor. Some …

1990790

…tests failing

All tests passing

ed51ca4

Removed editor from sequence_section type

66da0e5

Tests now want seq sec in range [0, numSections)

8aed7fb

SeqSec now 0-indexed. Removed last traces of editor field

e23466b

Replaced loops with iterators

b640012

More iter().enumerate()

043d496

rjayatilleka reviewed Aug 20, 2016
View reviewed changes

bookdude13 added 5 commits August 20, 2016 08:58

Updated README and fixed error in main

0e607d7

Removed unnecessary println

3e9d36f

PR tweaks

6a31815

Added checks

61892e0

Simplified getting all sections

fffeab0

bookdude13 closed this May 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resectioning sequences #35

Resectioning sequences #35

bookdude13 commented Aug 18, 2016

rjayatilleka Aug 19, 2016

bookdude13 Aug 19, 2016

rjayatilleka Aug 19, 2016

rjayatilleka commented Aug 19, 2016 •

edited

Loading

rjayatilleka commented Aug 19, 2016

rjayatilleka Aug 19, 2016

bookdude13 Aug 19, 2016

rjayatilleka commented Aug 19, 2016

rjayatilleka Aug 19, 2016 •

edited

Loading

bookdude13 Aug 19, 2016

rjayatilleka Aug 19, 2016

rjayatilleka Aug 19, 2016

rjayatilleka commented Aug 19, 2016

bookdude13 commented Aug 19, 2016

bookdude13 commented Aug 19, 2016

bookdude13 commented Aug 19, 2016

rjayatilleka Aug 20, 2016

rjayatilleka commented Aug 20, 2016

bookdude13 commented Aug 20, 2016

rjayatilleka commented Aug 25, 2016

bookdude13 commented Aug 25, 2016

bookdude13 commented Sep 19, 2016

Resectioning sequences #35

Resectioning sequences #35

Conversation

bookdude13 commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjayatilleka commented Aug 19, 2016 • edited Loading

rjayatilleka commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjayatilleka commented Aug 19, 2016

rjayatilleka Aug 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjayatilleka commented Aug 19, 2016

bookdude13 commented Aug 19, 2016

bookdude13 commented Aug 19, 2016

bookdude13 commented Aug 19, 2016

Choose a reason for hiding this comment

rjayatilleka commented Aug 20, 2016

bookdude13 commented Aug 20, 2016

rjayatilleka commented Aug 25, 2016

bookdude13 commented Aug 25, 2016

bookdude13 commented Sep 19, 2016

rjayatilleka commented Aug 19, 2016 •

edited

Loading

rjayatilleka Aug 19, 2016 •

edited

Loading