GNOME Bugzilla – Bug 701970
Updates to types of VC changes.
Last modified: 2014-05-24 21:04:22 UTC
Changes required for upcoming bzr changes
Created attachment 246469 [details] [review] Updates to types of VC changes. Add Renamed state to vc. Allow folders to have a modified state, and render them if so. Allow Dir types to have an options variable.
Each of these three changes is scary in its own right. Obviously I understand why you want/need the renamed state, but we're not going to do anything sensible with it (and can't, in our current model) so I'm not entirely convinced of the benefit. However, I'm sympathetic so I guess this will happen somehow. Can you explain the motivation behind the other two changes to me please?
Sorry for lacking descriptions. Renamed was planned on being used in bzr together with the options column on the File object to show the old name -> new name. Modified folder is because folders can be modified as well - as they are tracked in bzr. The options was added to Dir so we can use the options column for dirs as well. Given we track directories using bzr, if one is modified we should still display it in flattened mode, hense the vcview change. Happy to merge this patch together with the actual bzr changes which use them if you'd prefer? Or at least put them on the same bug I guess.
(In reply to comment #3) > Sorry for lacking descriptions. > > Renamed was planned on being used in bzr together with the options column on > the File object to show the old name -> new name. Right, but we can't do anything with the RENAMED state currently? I mean, I don't believe that we have the machinery in place to actually show a diff between the original repo version of a file and the moved version of the file. I think that without that, supporting moved files is kind of a moot point. > Modified folder is because folders can be modified as well - as they are > tracked in bzr. The options was added to Dir so we can use the options column > for dirs as well. Sorry, still want more details! :) What does it mean for a folder to be modified in bzr? and what do you actually want to put in the options column? > Given we track directories using bzr, if one is modified we should still > display it in flattened mode, hense the vcview change. Depending on the answers to the above, I guess I can see this... Do you know what happens if someone tried to activate a folder in flattened mode? and how about various VC actions (add/remove/revert/etc.)... have they been tested on modified folders? > Happy to merge this patch together with the actual bzr changes which use them > if you'd prefer? Or at least put them on the same bug I guess. Yeah I'd strongly prefer each of these changes to go into its own patch.
(In reply to comment #4) > (In reply to comment #3) > > Sorry for lacking descriptions. > > > > Renamed was planned on being used in bzr together with the options column on > > the File object to show the old name -> new name. > > Right, but we can't do anything with the RENAMED state currently? I mean, I > don't believe that we have the machinery in place to actually show a diff > between the original repo version of a file and the moved version of the file. > I think that without that, supporting moved files is kind of a moot point. My thinking here is more that the VC view should ideally show all uncommitted changes (or in future, all changes between revisions) Opening a diff for a renamed file works fine - if no changes it shows as identical, if changes they are shown as well (although the rename itself isn't shown, only back on the folder VC view) > > Modified folder is because folders can be modified as well - as they are > > tracked in bzr. The options was added to Dir so we can use the options column > > for dirs as well. > > Sorry, still want more details! :) > > What does it mean for a folder to be modified in bzr? and what do you actually > want to put in the options column? https://github.com/Psykar/meld/compare/bzr-changes probably explains best, but for directories it's a chmod +/- x > > > Given we track directories using bzr, if one is modified we should still > > display it in flattened mode, hense the vcview change. > > Depending on the answers to the above, I guess I can see this... Do you know > what happens if someone tried to activate a folder in flattened mode? and how > about various VC actions (add/remove/revert/etc.)... have they been tested on > modified folders? Activating a folder in flattened mode does nothing, there's no diff to show. > > > Happy to merge this patch together with the actual bzr changes which use them > > if you'd prefer? Or at least put them on the same bug I guess. > > Yeah I'd strongly prefer each of these changes to go into its own patch. Cleaning things up a bit now.
Created attachment 249994 [details] [review] Add states to version control. Adds renamed state for files and directories. Adds modified state for directories.
Created attachment 249995 [details] [review] Allow directories to have options column rendered.
Created attachment 249996 [details] [review] Rendered directories in flattened mode if required. Will render directories in flattened mode if their status is not normal. This allows for renamed directories, or permission changes on directories to be rendered.
Created attachment 249998 [details] [review] Rendered directories in flattened mode if required. Will render directories in flattened mode if their status is not normal. This allows for renamed directories, or permission changes on directories to be rendered.
Review of attachment 249998 [details] [review]: Changes have been made here actually, this patch needs more work still.
I've been reading your comments as "don't bother to look at this yet"... is that right? Just want to make sure you're not blocking on a response from me or anything.
Created attachment 251501 [details] [review] Rendered directories in flattened mode if required. Fixed!
Yep, IMO the three patches are ready now. Discussion and justifications above, short summary below. New states may be bzr specific but could be used by other VCs, even if opening them shows no differences (ie renamed has no content difference, but should still show in the folder view - IMO folder VC view should as much of the 'status' output as possible) Directories are tracked in bzr, as such should also allow to have stuff rendered in the options column, for renames or type changes. Because of the above, they should be rendered in flattened mode if they are not in the 'NORMAL' state, hense the 3rd patch. Due to other changes made since this patch was made, currently double clicking a folder in flattened mode, opens it in directory diff... with itself... not that helpful... but it's the same as what happens when you right click > compare when *not* flattened...
Created attachment 251502 [details] [review] Rendered directories in flattened mode if required. Erk. Was dev'ing on a new box, hadn't set up git properly yet. Patch is formatted properly now.
So generally, I've been hoping to stop the 1.7.x series and move on to getting 1.8 out the door and starting a GTK 3 port in earnest. The problem is that I need to be *really* convinced that these patches are low risk to put them in at this stage. The options column for folders seems fine. The RENAMED state is also low risk, though I do wonder how useful it is as a state on its own, since it's basically covering one of a possible multitude of metadata file changes. Would it be more useful to have a STATE_META_CHANGED or something? (not a rhetorical question... I'm really not sure) Also, RENAMED should probably not rendered as prominently as MODIFIED... maybe reuse the colour but remove the bold? The flattened change bothers me most, because that's actual behaviour change that doesn't only apply to new functionality. I'm also pretty sure it's wrong, as this will render folders as flattened if they're e.g., IGNORED, NONE or NOCHANGE. I don't think most VC backends use those, but still. Also, what happens if, for example, we have a NEW folder inside a MODIFIED folder? Reading the code I'm uncertain how that will actually render now in flattened mode.
Given these are precursors to more drastic changes to bzr I'm happy to wait for the next release then. Bzr in head is in an ok place anyway for now. If we can revisit for 1.8.x that's fine :-)
Created attachment 258388 [details] [review] Allow directories to have options column rendered.
Comment on attachment 258388 [details] [review] Allow directories to have options column rendered. Sorry, uploaded to wrong bug.
Created attachment 258389 [details] [review] Add states to version control. Adds renamed state for files and directories. Adds modified state for directories.
Comment on attachment 251502 [details] [review] Rendered directories in flattened mode if required. HEAD has been changed since this patch. Will open a separate bug for this one.
Can we look at this patch again? I've been running with it (and the subsequent bzr changes) for the last couple of months without issue.
Created attachment 258390 [details] [review] Rendered directories in flattened mode if required.
Created attachment 258391 [details] [review] Allow directories to have options column rendered.
It looks like these patches don't address things from comment 15 above? I'm also confused as to which patches are current I'm sorry, since you said that the flattened patch needed a new bug, but then updated it.
Sorry, I've been hard pressed for time. You're correct, a little more work needed to address comment 15
Revisiting this: An example of how things get displayed: http://i.imgur.com/UVE0nFe.png To cover points from comment 15 * Renamed state - this is a specific state in bzr and git: eg: $ echo "test" > test.txt $ git add test.txt $ git commit -m "Added stuff" ... $ git mv test.txt test2.txt $ git status On branch master Changes to be committed: (use "git reset HEAD <file>..." to unstage) renamed: test.txt -> test2.txt We can rename it (erk, meta) but I think renamed is fine as is. * Fair enough about removing the bold - easily done * Open to other suggestions about folder states - if we want to render them only if modified or renamed? * As above screenshot shows, a new folder inside a modified folder renders fine in flattened mode. We could show the modified/renamed folders only in non flattened mode though? I also had a look at the code surrounding expanding/collapsing folders vs. opening them in a new window. Currently the code will open a new comparison if the object in the treeview has no children. Obviously no items in flattened view have children, so folders would open in new windows if displayed in flattened mode as is. Without adding a new value to the tree specifying if something is a dir or not, I'm not sure this is possible?
(In reply to comment #26) > Revisiting this: > An example of how things get displayed: > http://i.imgur.com/UVE0nFe.png > > To cover points from comment 15 > > * Renamed state - this is a specific state in bzr and git: > > eg: > $ echo "test" > test.txt > $ git add test.txt > $ git commit -m "Added stuff" > ... > $ git mv test.txt test2.txt > $ git status > On branch master > Changes to be committed: > (use "git reset HEAD <file>..." to unstage) > > renamed: test.txt -> test2.txt Display looks good, excpet that I think we can probably spring for some unicode arrows instead of the "=>" ones. > We can rename it (erk, meta) but I think renamed is fine as is. > > * Fair enough about removing the bold - easily done > * Open to other suggestions about folder states - if we want to render them > only if modified or renamed? Yeah, let's reuse the modified colouring, minus the bold. We should also filter them state-wise as though they're modified I think. > * As above screenshot shows, a new folder inside a modified folder renders fine > in flattened mode. > We could show the modified/renamed folders only in non flattened mode though? No, I definitely think they should show in flattened mode. > I also had a look at the code surrounding expanding/collapsing folders vs. > opening them in a new window. Currently the code will open a new comparison if > the object in the treeview has no children. Obviously no items in flattened > view have children, so folders would open in new windows if displayed in > flattened mode as is. Without adding a new value to the tree specifying if > something is a dir or not, I'm not sure this is possible? Well, the tree stores the path, so you can always just os.path.isdir() it. I don't think there's a better way than that though. Also, 3.11 is well and truly underway, so getting these changes in soon would be good. Just let me know whenever you'd like me to look at patches. Incidentally, there's a recent change to not descending ignored folders that I *think* will work for bzr, but I haven't checked... just in case you notice anything.
Created attachment 276780 [details] [review] Add states to version control
Created attachment 276781 [details] [review] Rendered directories in flattened mode if required
Created attachment 276782 [details] [review] Bzr updates
Back again with some more time to push this properly. > > Display looks good, excpet that I think we can probably spring for some unicode > arrows instead of the "=>" ones. I tried adding ⇒ (unsure if it will render here) but even after adding the encoding to the python file (required for unicode, although I guess I could have just hex'd it up) it wouldn't render correctly in the list. Required to be pushed? > Yeah, let's reuse the modified colouring, minus the bold. We should also filter > them state-wise as though they're modified I think. Already done :) Fixed all other remaining issues AFAIK, and it's been rebased on current master. Only two patches to all vc stuff, bzr changes to use it (and show how it would work - happy to make a new bug for the bzr changes if required)
Review of attachment 276780 [details] [review]: A few tiny issues, but I'll happily push this with those fixes. ::: meld/tree.py @@ +99,3 @@ ("text-x-generic", "folder", new_fg, None), # NEW ("text-x-generic", "folder", mod_fg, None), # MODIFIED + ("text-x-generic", "folder", mod_fg, None), # RENAMED Can you align this please? @@ +121,3 @@ + icon = self.get_value(it, self.column_index(COL_ICON, pane)) + return icon == "folder" or os.path.isdir(path) + I don't have a better suggestion, and it's not a show-stopper, but... this is pretty ugly. This is to catch the case where the pane/path isn't actually identified as a folder because it doesn't exist? If so, could you make that a little clearer please? (I can see you've got the comment there anyway, but I don't think it describes the case well enough for me to remember in six months time.) Also, since this isn't used until the next patch, could you just move it there? ::: meld/vc/_vc.py @@ +32,3 @@ # error, placeholder, vc added # vc modified, vc conflict, vc removed +# locally removed, renamed, end This comment doesn't put the consts in the correct order. A leftover from an old patch maybe?
Review of attachment 276781 [details] [review]: Pretty much looks good to me. I'll push this once the previous patch is cleaned up.
(In reply to comment #31) > Back again with some more time to push this properly. > > > > Display looks good, excpet that I think we can probably spring for some unicode > > arrows instead of the "=>" ones. > > I tried adding ⇒ (unsure if it will render here) but even after adding the > encoding to the python file (required for unicode, although I guess I could > have just hex'd it up) it wouldn't render correctly in the list. Required to be > pushed? Definitely not required! I'm curious why it didn't work though... I may have a look at this if I get a chance. > Only two patches to all vc stuff, bzr changes to use it (and show how it would > work - happy to make a new bug for the bzr changes if required) Yeah other than those tiny fixes, I'm totally happy to apply those. As for the bzr changes, I might poke your test script a bit, but I'm pretty much going to trust you on these. I just don't have the time to torture test all of the VCs we support. Are you happy with this going in to 3.11.x? (I'm hoping really hard to get 3.11.1 out very soon, and then 3.12 shortly thereafter.)
Created attachment 277094 [details] [review] Add states to version control (bgo#701970)
Created attachment 277095 [details] [review] Render directories in flattened mode if required (bgo#701970)
Created attachment 277096 [details] [review] Bzr updates (bgo#701970)
Changes made - and updated the commit message format. I'm happy for this to go out in 3.11 I've been using these changes locally for close to a year now (my bad on taking so long) without issue across git or bzr. Might have a look at that unicode on the treeview not appearing correctly as well, hopefully it's something internal and doesn't require digging down into the gtk libraries.
Awesome, thanks. I've just pushed all of these to master, and added the unicode arrows. I'm not sure what problem you were seeing with the arrows, but they work fine for me; let me know if they still don't look right for you. While not a huge deal, the comparisons for moved files show the new file name in both panes. I think it would be better if they used the old file name where appropriate, but I'm not sure how much work that will be. Anyway, I'm closing this cause I think we're done here. Any new improvements we can put somewhere else. Thanks for pushing these patches forward.