qpush: Add preview button

[Craig Hewetson]

Is it possible to list all the changes that I have made (minus changes from other people via merges etc) in the qpush dialog. This is the "bzr diff -r ancestor:URL" command. Just to give the user an idea of what he or she is going to push to the server.

Senario:
I branch from a project.
I make changes A.
I merge with server (other people's changes B)
I then make some more changes C
and then i do a merge with server (with changes D)
now when I do commit and then use qpush to push. I would like to see a list of changes A and C. i.e my changes that i am contributing will be previewed before I push.

[Craig Hewetson (craighewetson-deactivatedaccount)]

Sorry instead qpushing doing a "bzr missing" it would be better if the user can execute a "qdiff -r ancestor:URL" instead. It will allow the user to see all his changes before he pushes.

[Craig Hewetson (craighewetson-deactivatedaccount)]

I took the time to add a button to the qpush dialog, called "preview changes" it will then open the diff window as it would when executing the command: "qdiff -r ancestor::push"

The changes can be found in my dev branch:

lp:~craighewetson/qbzr/dev

So if its wanted then the main developers can incorporate it into the trunk.

Also please comment on the changes since its all new to me.

[Craig Hewetson (craighewetson-deactivatedaccount)]

<updated the title and description>

[Craig Hewetson (craighewetson-deactivatedaccount)]

Proposed Implementation:

In QPush dialog,

Make the status group a tab (default focus), called "status"

Add an additional tab called "Changes" ("missing" might be a bit confusing)

The changes tab will contain the output of the bzr missing command.
The following options are available (as check boxes):
  --other Same as --theirs-only.
  --include-merges Show merged revisions.
  --mine-only Display changes in the local branch only.
  --show-ids Show internal object ids.
  --theirs-only Display changes in the remote branch only.

An addition push button must be added to this view to show the differences:
bzr qdiff -r ancestor:URL (where URL is the value in the location text combo)

NOTE: This must be implemented in such a way that it can be re-used in a future command called qmissing.

[Mark Hammond (mhammond)]

Initial comments:

* There are some spurious changes: specifically, readme.txt has an EOL only change, commit.py and main.py have some whitespace-only changes, statuscache.py has only a commented-out print, qbzr-merge.png has an unmentioned change. FYI, "bzr [q]diff -rancestor:..\path-to-trunk" is what I'm using to preview the changes...

* main.py adds imports for ChangeDesc and closure_in_selected_list, but neither are used.

* The new 'if checkedFiles...' block looks suspect - if not self.pending_merges, it appears checkedFiles will always be zero, so therefore cause the checkbox? Also, see Python's pep8 - it should probably be checked_files.

* Comments "# FIXME need to find a way to extract this from the commit.py file" and " # FIXME get this from INFOWindow ui" - any thoughts on them? Why are they hard (ie, why not include a patch for them too?)

I haven't previewed the UI changes, but it looks very promising :)

[Mark Hammond (mhammond)]

oops - please ignore "qbzr-merge.png has an unmentioned change" - I worked it out :)

[Craig Hewetson (craighewetson-deactivatedaccount)]

Hi Mark

I must sort out my EOL issues when it comes to bzr. (what IDE are you using? I'm currently using the pyDev eclipse plugin and i looked at Eric) Is there some way to configure EOL in bzr.

>>* The new 'if checkedFiles...' block looks suspect - if not self.pending_merges, it appears checkedFiles will always be zero, so therefore cause the checkbox? Also, see Python's pep8 - it should probably be checked_files.

I've noticed that too and fixed it in my bug fix branch: lp:~craighewetson/qbzr/trunk
I must look at the coding standards for python (coming from a java background I will have to watch myself)

>>FYI, "bzr [q]diff -rancestor:..\path-to-trunk" is what I'm using to preview the changes...
I was thinking of incorporating this into the qpush dialog as an action one can kick off. I think it would make sense if the user had the option to do this just before the final push.

>>* Comments "# FIXME need to find a way to extract this from the commit.py file" and " # FIXME get this from INFOWindow ui" - any thoughts on them?
Since i wanted to reuse a tab widget in the commit window, i tried to extract the set of widgets as a unit or blackbox. But the implementation wasn't encapsulated enough (made use of globals etc, that wasn't ). I think encapsulation must be in the back of the developers mind when making a gui component, since it could be reused in the future.

>>Why are they hard (ie, why not include a patch for them too?)
I've only started learning python about 2 weeks ago, and pyQT less than that. I only want to make that type of refactoring once I feel more comfortable with the languages.

[Craig Hewetson (craighewetson-deactivatedaccount)]

sorry a typo: (one of many)
>>(made use of globals etc, that wasn't ..
was trying to say:
The variables it used where not encapsulated enough....

[Mark Hammond (mhammond)]

> I must sort out my EOL issues when it comes to bzr.

Sorry, I wasn't clear - there don't seem to be "bad" EOL chars - just some trailing whitespace just before the EOL chars. They often sneak in, but doing a diff against the parent shows them, so I go back and fix them - similarly for 'accidental' blank lines I introduced. I like my diffs looking as small as possible :)

> (what IDE are you using?

I use komodo, but an editor like Scite is useful and has excellent whitespace and EOL capabilities.

> I've noticed that too and fixed it in my bug fix branch: lp:~craighewetson/qbzr/trunk
> I must look at the coding standards for python (coming from a java background I will have to watch myself)

Yeah - please google 'pep8'. If you change your branch to use 'checked_files' as the variable name I'd be happy to merge it. Sorry if it sounds picky :)

> >>FYI, "bzr [q]diff -rancestor:..\path-to-trunk" ...
> I was thinking of incorporating this into the qpush dialog

Sounds reasonable!

> Since i wanted to reuse a tab widget in the commit window,
> i tried to extract the set of widgets as a unit or blackbox.

Yeah, that sounds worthwhile - although you may find its better to encapsulate the *behaviour* of the widgets rather than the actual widgets and their layout, as each form might have subtly different layout semantics. The 'WorkingTreeFileList' widget is an example of this, but I can see how it does makes sense when talking about tabs.

However - I can't see any particular problems with the implementation in commit.py that would prevent refactoring. What issue with globals are you referring to?

> I've only started learning python about 2 weeks ago, and pyQT less than that.

Awesome - excellent work :) Its probably worthwhile to move towards this goal anyway to reduce the duplication - we will be more than happy to help.

BTW - what branch is this being tracked in now - lp:~craighewetson/qbzr/push-missing ?

[Craig Hewetson (craighewetson-deactivatedaccount)]

>>pep8
I'll take a look at that Style Guide for Python and I will update my (bugfix branch) lp:~craighewetson/qbzr/trunk to make use of the best practices they prescribe. Although I think the qbzr developers already merged it into the main dev branch. But if you come across those bad names in my changes you are welcome to change them.

>>BTW - what branch is this being tracked in now - lp:~craighewetson/qbzr/push-missing ?
Yes. But its a work in progress :) ... getting some memoryError when accessing a string, I'll figure out what I'm doing wrong.

I'll follow that same approach when removing white space differences. Its weird though because I don't have such a major problem when I work with java, it might be that my editor isn't being wise when it comes to changes in white space.

Another question:
Don't you that the py files are a bit over loaded with classes. In other words there are too many classes in the same file. Example: pull.py has push, branch and other ("slighty" unrelated) classes. Won't it be better to create a subdirectory under lib and then add separate files for each class under that directory. (this might be too java'ish :) )

[Mark Hammond (mhammond)]

I agree - pep8 would also. I think we are just talking about the natural evolution of the project though - sometimes things seem trivial when started, so merging functionality into one module is appealing, then (very slightly) regretted :)

I should also note that AFAIK, noone has so far insisted qbzr use pep8 - the bzr team has for bzr itself, but that doesn't mean I have the right to make the same decision for qbzr. OTOH, its a safe default :)

Cheers

[Lukáš Lalinský (luks)]

Dňa Ut, 2008-11-11 o 07:53 +0000, Mark Hammond napísal:
> I agree - pep8 would also. I think we are just talking about the
> natural evolution of the project though - sometimes things seem trivial
> when started, so merging functionality into one module is appealing,
> then (very slightly) regretted :)
>
> I should also note that AFAIK, noone has so far insisted qbzr use pep8 -
> the bzr team has for bzr itself, but that doesn't mean I have the right
> to make the same decision for qbzr. OTOH, its a safe default :)

The thing is that Qt uses camel case, while pep8 (and therefore bzrlib)
uses underscores, so it will be messy either way. :) But most of the new
code follows pep8, I think we should stick with that.

Lukas

[Craig Hewetson (craighewetson-deactivatedaccount)]

I'm thinking about making use of a preview button on the qpush dialog (executing bzr qdiff -rancestor:URL). This will preview the changes that will be push as a result of the user's direct changes, in the diff window.

Should this button be located at the bottom left hand corner? (where "Diff" button is located in qcommit)

NOTE:
The qmerge dialog will probably also have a preview button (since it supports --preview)

[Alexander Belchenko (bialix)]

Craig, do you have the patch somewhere for this problem?

[Craig Hewetson (craighewetson-deactivatedaccount)]

I no longer have the branch on my system and I removed it from launchpad. So you are correct in removing the has-patch tag