qpush: Add preview button

Bug #295130 reported by Craig Hewetson
2
Affects Status Importance Assigned to Milestone
QBzr
Confirmed
Medium
Unassigned

Bug Description

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.

Tags: feature
description: updated
Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote : Re: A Qmissing implementation in the Qpush dialog

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.

Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote :

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.

Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote : Re: A way to preview of changes before pushing in the Qpush dialog

<updated the title and description>

description: updated
Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote :

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.

Revision history for this message
Mark Hammond (mhammond) wrote :

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 :)

Revision history for this message
Mark Hammond (mhammond) wrote :

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

Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote :

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.

Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote :

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....

Revision history for this message
Mark Hammond (mhammond) wrote :

> 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 ?

Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote :

>>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 :) )

Revision history for this message
Mark Hammond (mhammond) wrote :

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

Revision history for this message
Lukáš Lalinský (luks) wrote : Re: [Bug 295130] Re: A way to preview of changes before pushing in the Qpush dialog

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

Changed in qbzr:
importance: Undecided → Wishlist
status: New → Confirmed
Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote : Re: A way to preview of changes before pushing in the Qpush dialog

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)

tags: added: feature
removed: feature-request
summary: - A way to preview of changes before pushing in the Qpush dialog
+ qpush: Add preview button
tags: added: has-patch
Changed in qbzr:
importance: Wishlist → High
Revision history for this message
Alexander Belchenko (bialix) wrote :

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

tags: removed: has-patch
Revision history for this message
Craig Hewetson (craighewetson-deactivatedaccount) wrote :

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

Changed in qbzr:
importance: High → Medium
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.